Skip to content

Issue #16836: New Check HexLiteralCase #17233

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #16836: New Check HexLiteralCase #17233

wants to merge 1 commit into from
+729 −58

Conversation

zyadahmed
Copy link
Contributor

@zyadahmed zyadahmed commented Jun 16, 2025 •
edited
Loading

Issue: #16836
New HexLiteralCase check that verifies hexadecimal number literals contain only uppercase letters (A–F). If any lowercase letters are found, a violation is raised with the message: Should use uppercase hexadecimal digit instead of ''{0}'' where {0} is replaced with that violated digits.
Contribution PR link: checkstyle/contribution#950

romani
romani requested changes Jun 22, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

thanks a lot,

Please read and watch videos at Starting_Development.
Please make CI green. There are videos on CI handling.

items:

@zyadahmed zyadahmed force-pushed the HexLiteralCase branch 4 times, most recently from 91b3b7a to 6bcc5b5 Compare June 24, 2025 21:24
@romani
Copy link
Member

romani commented Jun 25, 2025

All CIs should be green.
Please reply "done" to each comment as you finished addressing it

@zyadahmed zyadahmed mentioned this pull request Jun 25, 2025
Open
@zyadahmed zyadahmed force-pushed the HexLiteralCase branch 4 times, most recently from d0fe5c9 to 855949d Compare June 25, 2025 22:32
@zyadahmed
Copy link
Contributor Author

All CIs should be green. Please reply "done" to each comment as you finished addressing it

for the ci/semaphore created pr in checkstyle/contribution and added the pr link in description is that the correct way or am i missing something ?

@romani
Copy link
Member

romani commented Jun 27, 2025 •
edited
Loading

This single failure is expected for new Check.
This is unfortunate until we fix #7304

@romani
Copy link
Member

romani commented Jun 27, 2025

Please move to next phase, testing on real sources.

Please review #16946 , as example of how user put config in description of PR, and triggered execution by comment #16946 (comment)

romani
romani requested changes Jun 27, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

src/main/java/com/puppycrawl/tools/checkstyle/checks/HexLiteralCaseCheck.java
* <div>Checks that hexadecimal literals are defined using uppercase letters {@code (A-F)}
* rather than lowercase {@code (a-f)}. This follows the
* <a href="https://cr.openjdk.org/~alundblad/styleguide/index-v6.html#toc-literals">
* OpenJDK Style Guide</a>.
Copy link
Member

Choose a reason for hiding this comment

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

We can not rely on link for description.
It might be broken soon, we need to copy all details on how bit work to Check description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/test/java/com/puppycrawl/tools/checkstyle/checks/HexLiteralCaseCheckTest.java Outdated

@Test
public void testCheck()
throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Move this method to the end of class , to group together technical coverage of token method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...esources/com/puppycrawl/tools/checkstyle/checks/hexliteralcase/InputHexLiteralCaseCheck.java Outdated
int a = 0xABC;
int k = 0XCAFEBAB1;
long d = 0Xf123L; // violation 'Should use uppercase hexadecimal letters.'
int binaryValue = 0b1010;
Copy link
Member

Choose a reason for hiding this comment

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

For each line with violation please add kind of same line but without violation, that would be result of what vis expected refactoring from violated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...esources/com/puppycrawl/tools/checkstyle/checks/hexliteralcase/InputHexLiteralCaseCheck.java
int binaryValue = 0b1010;
int num = 1;


Copy link
Member

Choose a reason for hiding this comment

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

Please add way more items to testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...xdocs-examples/resources/com/puppycrawl/tools/checkstyle/checks/hexliteralcase/Example1.java Outdated
class Example1 {
int a = 0xabc; // violation 'Should use uppercase hexadecimal letters.'
int b = 0xABC;
long l = 0Xf123L; // violation 'Should use uppercase hexadecimal letters.'
Copy link
Member

Choose a reason for hiding this comment

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

All from examples guidelines should be as examples in our website

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

All from issue description, should be in example.

src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionHexLiteralCaseTest.java
runVerifications(config, fileProcess, expected, expectedXpathQueries);
}

}
Copy link
Member

Choose a reason for hiding this comment

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

Please add one more, we usually ask for 3 tests for xpath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@zyadahmed zyadahmed force-pushed the HexLiteralCase branch 2 times, most recently from 39473cd to 647dc3f Compare June 27, 2025 12:10
@zyadahmed
Copy link
Contributor Author

github, generate report

1 similar comment
@zyadahmed
Copy link
Contributor Author

github, generate report

@romani
Copy link
Member

romani commented Jun 27, 2025

You need to put link to config in PR description .
Please read https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#generation-examples
New module config:

@zyadahmed
Copy link
Contributor Author

GitHub, generate report

@zyadahmed
Copy link
Contributor Author

Github, generate report for configs in PR description

@github-actions GitHub Actions
Copy link
Contributor

Report generation failed. Please check the logs for more details.

Link: https://github.com/checkstyle/checkstyle/actions/runs/15948633638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zyadahmed @romani