Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checked the length of key for checker framework #583

Merged
merged 2 commits into from Feb 2, 2021
Merged

Conversation

ek08
Copy link
Contributor

@ek08 ek08 commented Jan 27, 2021

Checked the length of key IN JSONObject for Checker Framework

@stleary
Copy link
Owner

stleary commented Jan 28, 2021

What problem does this code solve?
JavaCheck fails due to not being able to detect that zero-length keys are disallowed. I don't think there is any change to functionality.

Risks
Low

Changes to the API?
No

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
No

Review status
APPROVED - after resolution of comments.

Starting 3-day comment window.

@johnjaylward
Copy link
Contributor

I don't see what this change is adding to the code. The key can not be 0 length at that spot. Also, I would argue that the logic chosen for the check is wrong.

I'd argue it should look like this if you feel the check needs to be added

if (key.length() == 0 || Character.isLowerCase(key.charAt(0))) {
    return null;
}

Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check is not valid

@ek08
Copy link
Contributor Author

ek08 commented Jan 28, 2021

I don't see what this change is adding to the code. The key can not be 0 length at that spot. Also, I would argue that the logic chosen for the check is wrong.

I'd argue it should look like this if you feel the check needs to be added

if (key.length() == 0 || Character.isLowerCase(key.charAt(0))) {
    return null;
}

Thanks for the correction. I agree with you that the length of key cannot be 0 length at that spot. So the logic of the check should be changed to

if (key.length() == 0 || Character.isLowerCase(key.charAt(0)))

@stleary
Copy link
Owner

stleary commented Jan 30, 2021

Approval restored.

This was referenced Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants