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

NumericLiteralNeedsUnderscore's new option to skeep variables by name #425

Closed
romani opened this issue Jan 18, 2016 · 10 comments
Closed

NumericLiteralNeedsUnderscore's new option to skeep variables by name #425

romani opened this issue Jan 18, 2016 · 10 comments
Milestone

Comments

@romani
Copy link
Member

@romani romani commented Jan 18, 2016

all details : https://travis-ci.org/checkstyle/checkstyle/jobs/103024859

code coverage should be 100%, so line https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L134 should be removed.

@romani
Copy link
Member Author

@romani romani commented Jan 18, 2016

@cypai , please address this ASAP.

please generate reports (by chekcstyle-tester project) over other projects to make sure there are no other code that need to be ignored.

@cypai
Copy link
Contributor

@cypai cypai commented Jan 19, 2016

Will be done this week

@cypai
Copy link
Contributor

@cypai cypai commented Jan 24, 2016

@romani
I think exactly which variables to be ignored merits some discussion. Like, should only variable declarations be ignored? For example:

public static final long serialVersionUID = 1234567890L;  // This is ignored

public void bar() {
  long foo = 1234567890L;  // This is ignored
  foo -= 1234567;  // What about this?
  foo = 1234567;  // What about this?
}

(By the way, you really need to fix your formatter for eclipse, I needed to fix indentation and other whitespace problems manually because it didn't behave correctly, and it wasn't fun when saving caused everything to mess up again)

@romani
Copy link
Member Author

@romani romani commented Jan 24, 2016

We need to ignore variable or field that could have generated value. For now we know only about fields that could have generated fields that people usually do not care but that used in massive amount of code.

For now, lets make ignore option only for field names, serialVetsionID is default value.

After update please run Check against some opensource code to make sure it works and nothing new need to be created. You can use contribution/checkstyle-tester project to generate html report.

@cypai
Copy link
Contributor

@cypai cypai commented Jan 29, 2016

From discussion:
Ignore configuration should be called ignoreFieldNamePattern. It should be done as a Regexp.
These should only care about class fields; if a number is used in a function then it is significant and should be checked.
Reference the main checkstyle project for examples of getting fields.

Afterwards, run it on Spring, Guava, and Orekit to see what comes up.

@romani
Copy link
Member Author

@romani romani commented Jan 29, 2016

default value of ignoreFieldNamePattern is "^serialVersionUID$" .

Try to run your Check at your private projects.

@romani
Copy link
Member Author

@romani romani commented Feb 20, 2016

@cypai , please finish this issue.

cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 5, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 5, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 6, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 6, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 6, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 6, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 7, 2016
@romani romani added this to the 1.19.0 milestone Mar 7, 2016
@romani
Copy link
Member Author

@romani romani commented Mar 7, 2016

@cypai , please send PR checkstyle repo to start usage of this Check.
I will do release of sevntu tomorrow.

FYI: All senvtu Checks have to be used in main repo - checkstyle. config - https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_sevntu_checks.xml

@romani
Copy link
Member Author

@romani romani commented Mar 7, 2016

@cypai ,

New option is introduces but eclipse-cs/sonar/default-config are not updated:

sevntu.checkstyle/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml
sevntu.checkstyle/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.xml

New option need to be added here too with its default value (as it is example of usage of all properties):
sevntu.checkstyle/gh-pages/sevntu-checkstyle-default-configuration.xml

Without such update, release is useless.
Please confirm that Elcipse-cs and Sonar works fine on new configurations, property change is working fine.

cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 7, 2016
cypai added a commit to cypai/sevntu.checkstyle that referenced this issue Mar 7, 2016
romani added a commit that referenced this issue Mar 7, 2016
Issue #425: Update default configuration for NumericLiteralNeedsUnderscoreCheck
@romani
Copy link
Member Author

@romani romani commented Mar 7, 2016

fixes are merged.

@romani romani closed this Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants