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

Issue #8764: Create new check IllegalIdentifierName #8766

Merged

Conversation

@nrmancuso
Copy link
Member Author

Github, generate report

@nrmancuso nrmancuso force-pushed the full-records-IllegalIdentifierName branch from 3b6cb1b to a443917 Compare August 27, 2020 14:03
@nrmancuso
Copy link
Member Author

Github, generate report

@romani
Copy link
Member

romani commented Aug 27, 2020

from wercker:

[INFO] --- maven-checkstyle-plugin:3.1.1:check (check) @ patch-diff-report-tool ---
[INFO] There are 4 errors reported by Checkstyle 8.36-SNAPSHOT with ../../../config/checkstyle_checks.xml ruleset.
[ERROR] src/main/java/com/github/checkstyle/parser/CheckstyleTextParser.java:[176,36] (naming) IllegalIdentifierName: Name 'record' must match pattern '(?i)^(?!(record|yield|var)$).+$'.
[ERROR] src/main/java/com/github/checkstyle/parser/CheckstyleTextParser.java:[222,32] (naming) IllegalIdentifierName: Name 'record' must match pattern '(?i)^(?!(record|yield|var)$).+$'.
[ERROR] src/main/java/com/github/checkstyle/data/DiffReport.java:[132,31] (naming) IllegalIdentifierName: Name 'record' must match pattern '(?i)^(?!(record|yield|var)$).+$'.
[ERROR] src/main/java/com/github/checkstyle/site/SiteGenerator.java:[166,35] (naming) IllegalIdentifierName: Name 'record' must match pattern '(?i)^(?!(record|yield|var)$).+$'.

please fix indentation problem in html - https://nmancus1.github.io/site_illegalidentifiername/config_naming.html#IllegalIdentifierName for configs.

@strkkk
Copy link
Member

strkkk commented Aug 27, 2020

@nmancus1 I suggest to extend list of illegal identifiers-keywords with '_' (underscore) - https://stackoverflow.com/questions/23523946/underscore-is-a-reserved-keyword

FYI for new module report you should not use patch report generation, but new module link prefixes instead, it is clearer. https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#basic-single-report

@nrmancuso
Copy link
Member Author

@strkkk

I suggest to extend list of illegal identifiers-keywords with '_' (underscore) - https://stackoverflow.com/questions/23523946/underscore-is-a-reserved-keyword

This is a good suggestion, thanks.

FYI for new module report you should not use patch report generation, but new module link prefixes instead, it is clearer. https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#basic-single-report

Thank you, that is a good point. Should I amend and re-run report generation?

@strkkk
Copy link
Member

strkkk commented Aug 27, 2020

Thank you, that is a good point. Should I amend and re-run report generation?

I don't think it is necessary in this case, it is for future usages.

@nrmancuso
Copy link
Member Author

nrmancuso commented Aug 27, 2020

I suggest to extend list of illegal identifiers-keywords with '_' (underscore)

I misunderstood this at first, I am updating this PR now.

Edit: _ is now an illegal identifier name.

@nrmancuso nrmancuso force-pushed the full-records-IllegalIdentifierName branch from 2f13cea to 4e3d03d Compare August 27, 2020 20:22
@nrmancuso
Copy link
Member Author

Site is updated.

@romani
Copy link
Member

romani commented Aug 27, 2020

Github, generate report

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.

Minor:

config/checkstyle_checks.xml Outdated Show resolved Hide resolved
@nrmancuso nrmancuso force-pushed the full-records-IllegalIdentifierName branch from 4e3d03d to 2cdbb56 Compare August 27, 2020 21:14
@nrmancuso nrmancuso force-pushed the full-records-IllegalIdentifierName branch from 2cdbb56 to cb1e152 Compare August 28, 2020 00:00
@romani
Copy link
Member

romani commented Aug 28, 2020

codecov could be ignored.
We lost coverage % a bit, by metadata PR merge, that is why it might be messing with math of %.

While jacoco and pitest are happy, codecov can be ignored.

Copy link
Member

@pbludov pbludov left a comment

Choose a reason for hiding this comment

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

two minor changes please

@nrmancuso nrmancuso force-pushed the full-records-IllegalIdentifierName branch from cb1e152 to 0a02fb6 Compare August 28, 2020 13:07
@pbludov pbludov merged commit 5da2631 into checkstyle:master Aug 28, 2020
@pbludov pbludov linked an issue Aug 28, 2020 that may be closed by this pull request
@nrmancuso nrmancuso deleted the full-records-IllegalIdentifierName branch August 28, 2020 18:52
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.

Create new check IllegalIdentifierName
4 participants