Skip to content

Checked the length of key for checker framework#583

Merged
stleary merged 2 commits into
stleary:masterfrom
ek08:fix
Feb 2, 2021
Merged

Checked the length of key for checker framework#583
stleary merged 2 commits into
stleary:masterfrom
ek08:fix

Conversation

@ek08
Copy link
Copy Markdown
Contributor

@ek08 ek08 commented Jan 27, 2021

Checked the length of key IN JSONObject for Checker Framework

@stleary
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants