-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add support to ignore JSNI methods for Google style config #14487
Comments
Me and @romani discussed this before in gitter chat. The discussion was focused on Google Java Style Guide, in that we needed to ignore the length of JSNI methods. He suggested me to use the suppression but I guess my way will be more convenient. Here is the thread link to our conversation: link.I would be happy to hear what other maintainer's think about how we should approach this. |
Let's restate issue, from CLI execution should show usage of Google config file. Expected no violations. |
@Zopsss please link to where this is described in the Google Style guide. |
Here it is:
Checkstyle google checks doc:
|
These details should be in the issue description, please place them there. This looks like it is solvable by https://checkstyle.org/filters/suppresswithplaintextcommentfilter.html#SuppressWithPlainTextCommentFilter Inspired by my example at #11867 (comment):
|
Done
Yeah this can be solved using this filter but I was thinking if maybe some user wants to ignore JSNI method's length then he need to compulsory use the filter, there is no property to set to ignore the JSNI methods. If there is some property available then it will be less overhead for user to achieve this, that's my point. |
We won’t be adding a new property for something like this; new properties add complexity and maintenance overhead. Issue is easily solved by filter. Please update the issue description to have the premise described at #14487 (comment). We would only be adding this filter to google_checks.xml. |
Okayy got it! I'll do it once I get back on my laptop. |
sorry for the late reply, I have updated the issue description as you both suggested. @romani @nrmancuso can you check that out pls? |
Let's make sure we have an agreement on the scope of the work here, I am not 100% familiar with all the nuances of our google style guide coverage. I think we need to:
@Zopsss can you find some other PRs with some updates like this and see what was done there? @romani anything else you can think of to add here? |
Yeah, we will need to update the documentation, adding examples of JSNI methods to indicate that we now also support ignoring JSNI methods, we will need to write a test case to ensure that this functionality will be consistent in future and of course we will need to add suppression to our google config near LineLength check. I agree with all your points 👍
okay I will try to find it and will update you as soon as possible. |
Yes, plus: |
okay I will do it once I start working on this issue. |
@nrmancuso I found this PR in which support for validating import and package statement for LineLength check were added. The contributor has updated the check itself to add support and also made test cases to ensure the new functionality is working fine, he updated the doc based on the new functionality and also updated the google check config respectively. This is similar to our case but we don't need to change the check implementation and it's documentation as we're only adding suppression in our google_checks.xml, we will only need to update the 4.4 Column Limit: 100 section of Checkstyle's google java style coverage documentation and write a google style test to ensure everything is working fine. And of course we need to add the suppression near LineLength check and write a comment explaining why it is there. |
I guess now we have a clear goal to work on. Can I start working on this issue? @romani |
We need to create new "it" tests in this chapter, similar to what was completed in https://github.com/checkstyle/checkstyle/pull/4149/files. |
@Zopsss I have updated the issue description to reflect what we expect to be done here, please send a PR |
Okay but I have a doubt.... Why do we write some test cases in |
to be sure that jav code written in Input file has violation or has no violation, and does not matter how much Checks/filters make it possible. |
sorry but can you explain it in more detail? I'm still not able to understand it properly |
https://github.com/checkstyle/checkstyle/tree/master/src/test this is testing of each module (checks, filters ) completely independently. Inputs covers all possible permutations of Check properties. https://github.com/checkstyle/checkstyle/tree/master/src/it/java/com this is testing of config file ( combination of multiple Checks/filters), for now we have 2 config ( Google and Sun). So Inputs are good and not good for Google style. |
…ngth check in google config
okay and what is the use of https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle ? Specifically this directory: https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter I wanted to know the reason behind using the |
|
got it! thanks for the explanation :) |
…ngth check in google config
…ngth check in google config
…ngth check in google config
…ngth check in google config
…ngth check in google config
…ngth check in google config
…ngth check in google config
Closing this issue, reason mentioned here: #14938 |
I have read check documentation: https://checkstyle.org/checks/sizes/linelength.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words
Problem Statement:
We need to ignore JSNI methods' length based on the 4.4 Column Limit: 100 rule of Google Java Style Guide.
This is also defined at Checkstyle's Google Check Doc: https://checkstyle.org/google_style.html#a4.4
Currently Checkstyle google config does not ignore the JSNI method's LineLength. We need to add suppression to ignore JSNI methods.
Current behaviour:
config.xml
google_checks.xml
Test.java
Output:
Describe what you expect in detail.
If we don't want to have WARNING message logged here, we need to use a suppression filter to do that. We can use SuppressWithPlainTextCommentFilter filter to ignore JSNI methods as other maintainer suggested below. The suppression might look something like this:
Expected output:
To be completed in this issue:
it
folder, in this specific chapterThe text was updated successfully, but these errors were encountered: