Issue #14487: add support to ignore JSNI methods for LineLength check in google config #14501

Zopsss · 2024-02-18T07:54:54Z

Ignore JSNI methods' length in google config based on 4.4 Column Limit: 100

Exceptions:

Lines where obeying the column limit is not possible (for example, a long URL in Javadoc, or a long JSNI method reference).

romani

please update

checkstyle/src/xdocs/google_style.xml

Lines 790 to 793 in a4411aa

    
           <a href="https://webtoolkit.googleblog.com/2008/07/getting-to-really-know-gwt-part-1-jsni.html"> 
        
             JSNI</a> 
        
           could not be detected right now, but might be possible after 
        
           comments and javadoc support appear in Checkstyle.

items:

src/main/resources/google_checks.xml

.../google/checkstyle/test/chapter4formatting/rule44columnlimit/InputLineLengthJsniMethods.java

src/it/java/com/google/checkstyle/test/chapter4formatting/rule44columnlimit/LineLengthTest.java

Zopsss · 2024-02-18T18:54:23Z

sorry I accidently requested a re-review from you.. I don't have permission to dismiss it

Zopsss · 2024-02-18T19:01:26Z

mvn clean verify is giving these errors:

[ERROR] Failures: 
[ERROR]   XdocsPagesTest.testAllCheckSections:579->validateCheckSection:665->validateUsageExample:1545 suppresswithplaintextcommentfilter.xml section 'SuppressWithPlainTextCommentFilter' should have a google section since it is in it's config
expected to be true
[ERROR]   XdocsPagesTest.testAllStyleRules:1672 google_style.xml requires the following check(s) to appear: [SuppressWithPlainTextCommentFilter]
expected to be empty
but was: [SuppressWithPlainTextCommentFilter]

I have added the suppression description in google_style.xml but it is still complaining about it. What do I need to do to fix this?

And also I added following to the suppresswithplaintextcommentfilter.xml in Example of Usage subsection:

<li>
  <a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources+path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWithPlainTextCommentFilter">
  Google Style</a>
</li>

but this gets automatically removed after mvn clean verify

note: the link I specified in href is not working currently as there is no SuppressWithPlainTextCommentFilter in Checkstyle's main branch's google_checks.xml

Zopsss · 2024-02-22T03:54:59Z

@romani ping

nrmancuso · 2024-02-23T04:00:18Z

suppresswithplaintextcommentfilter.xml

This is generated from the associated template file, please update that.

I have added the suppression description in google_style.xml but it is still complaining about it. What do I need to do to fix this?

Please use git blame to find some commit where we added other filters, there might be some special changes needed for these.

Zopsss · 2024-02-23T18:31:56Z

suppresswithplaintextcommentfilter.xml

This is generated from the associated template file, please update that.

I have added the suppression description in google_style.xml but it is still complaining about it. What do I need to do to fix this?

Please use git blame to find some commit where we added other filters, there might be some special changes needed for these.

thanks for the help, I have resolved these errors.

I'm facing some new errors after running mvn clean verify now...

org.apache.maven.project.ProjectBuildingException: Error resolving project artifact: org.eclipse.m2e:lifecycle-mapping:pom:1.0.0 was not found in https://repo.maven.apache.org/mave
n2 during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of central has elapsed or updates are forced for project org.eclipse.m2e:lifecycle-mapping:pom:1.0.0

Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: org.eclipse.m2e:lifecycle-mapping:pom:1.0.0 was not found in https://repo.maven.apache.org/maven2 during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of central has elapsed or updates are forced

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project checkstyle: Error generating maven-javadoc-plugin:3.6.3:javadoc report: Project contains Javadoc Warnings -> [Help 1]

some warning classes mentioned in the last errors are:

[WARNING] C:\Open Source\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\gui\BaseCellEditor.java:38: warning: use of default constructor, which does not provide a comment 
[WARNING] public class BaseCellEditor implements CellEditor {
[WARNING] ^
[WARNING] C:\Open Source\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\filefilters\BeforeExecutionExclusionFileFilter.java:60: warning: use of default constructor, which does not provide a comment

note: It mentioned so many warning classes, I did not add all of them otherwise the comment would be too long... I also tried to run mvn clean site it was also giving me same errors.

I did not touched these classes at all, I think the error is not caused by the changes I made, it is caused by something else... I'm not so sure what to do with these errors.

Can you help me out with this? @nrmancuso

nrmancuso · 2024-02-23T20:29:25Z

I did not touched these classes at all, I think the error is not caused by the changes I made, it is caused by something else

You can always check this by checking out master and running mvn clean verify on your local and comparing that to this branch.

Zopsss · 2024-02-24T17:51:08Z

You can always check this by checking out master and running mvn clean verify on your local and comparing that to this branch.

IDK how but now mvn clean verify gives BUILD SUCCESS but the mvn clean site is still giving the same warning error in both branches, master and the branch im working on.

Zopsss · 2024-02-26T15:40:36Z

You can always check this by checking out master and running mvn clean verify on your local and comparing that to this branch.

IDK how but now mvn clean verify gives BUILD SUCCESS but the mvn clean site is still giving the same warning error in both branches, master and the branch im working on.

@nrmancuso @romani is my PR ready to merged?

Zopsss · 2024-02-27T16:46:08Z

@nrmancuso did the git pull and also rebased my branch with master but it is still giving the same error... maybe some merged PR introduced this bug.

and sorry for the late reply, GitHub didn't notified me about the force push

nrmancuso · 2024-02-28T04:24:45Z

@Zopsss spellcheck failure is valid, please fix it

Zopsss · 2024-02-28T16:43:41Z

@Zopsss spellcheck failure is valid, please fix it

done. I also needed to modify releases.xml as it was giving an error because a line was longer than 100 characters. @nrmancuso

Zopsss · 2024-02-28T17:46:21Z

dont know why circleci and semaphoreci are complaining, they didn't specify proper error message ;__;

nrmancuso · 2024-02-28T21:18:27Z

@Zopsss please resolve conflicts

Zopsss · 2024-02-29T04:53:34Z

@Zopsss please resolve conflicts

Done and now all checks are passing expect the Cirrus @nrmancuso

github-actions · 2024-02-29T10:30:10Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/1b4814d_2024102732/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/1b4814d_2024102732/filters/suppresswithplaintextcommentfilter.html#Example_of_Usage

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/1b4814d_2024102732/google_style.html#Google_Coverage_table

Zopsss · 2024-02-29T10:38:33Z

the website looks fine to me. I guess we're good to go now

Zopsss · 2024-03-02T04:30:45Z

@nrmancuso

nrmancuso

Items:

src/xdocs/google_style.xml

.../google/checkstyle/test/chapter4formatting/rule44columnlimit/InputLineLengthJsniMethods.java

…ngth check in google config

nrmancuso

@Zopsss thanks a lot, it is always good to see more ✅ on the google config!

Zopsss · 2024-03-08T19:08:25Z

@romani ping

romani · 2024-03-12T04:38:32Z

GitHub, generate website

romani

Ok to merge

github-actions · 2024-03-12T04:46:10Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/40934dd_2024044506/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/40934dd_2024044506/filters/suppresswithplaintextcommentfilter.html#Example_of_Usage

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/40934dd_2024044506/google_style.html#Google_Coverage_table

romani · 2024-03-12T12:44:51Z

the only question can be:
Do we need to add SuppressWithPlainTextCommentFilter to table https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/40934dd_2024044506/google_style.html#a4.4 ? as it become very meaningful part of coverage.
Other filters are not added, so it is gray area.

Zopsss · 2024-03-12T13:53:09Z

Other filters are not added, so it is gray area.

Umm I'm not so sure about it because there are other Checks too which uses suppressions but we've not added them to the table.

For example,

checkstyle/src/main/resources/google_checks.xml

Line 109 in 40934dd

we're using SuppressionXpathSingleFilter for RightCurlyAlone property but we've not added it to the table. Here we're using suppression on a property of the check but in our case we're using the suppression on our whole Check.

Maybe we can add the suppression to the Suppressions section, like I did here #14501 (review), I've removed thay code as Mr Nick suggested but if you agree with me then I can add it again. Again I'm not so sure that we should follow this approach or we should do as you suggested.

romani · 2024-03-12T14:43:11Z

We can always improve in separate issue. If maintainers find it useful. I am not sure about it.

rnveach · 2024-03-13T04:12:20Z

src/main/resources/google_checks.xml

+  <!-- See https://google.github.io/styleguide/javaguide.html#s4.4-column-limit  -->
+  <module name="SuppressWithPlainTextCommentFilter">
+    <property name="offCommentFormat" value="\/\*-\{"/>
+    <property name="onCommentFormat" value="\}-\*\/"/>


@nrmancuso @romani Why are we ignoring the entire JSNI method?

https://google.github.io/styleguide/javaguide.html#s4.4-column-limit

Lines where obeying the column limit is not possible (for example, ... a long JSNI method reference)

Google seems pretty clear it should only be the method reference, in the issue's case seems to be the method declaration which I am not sure it is a reference, and not the entire method.

The console.log violation in the issue seems valid to me. I am starting to think the other violation is valid.

https://checkstyle.org/google_style.html#a4.4

JSNI could not be detected right now, but might be possible after comments and javadoc support appear in Checkstyle.

We cannot detect JSNI references without parsing support IMO.

If we are to ignore JSNI completely because it is not supported by us, then https://checkstyle.org/google_style.html#a4.4 needs to be updated to say that and shouldn't be removed or turned green. The example in the issue doesn't seem to be a valid reason to ignore JSNI. It should be centered around we cannot identify JSNI method references which is what Google says we must be able to identify.

Google seems pretty clear it should only be the method reference

Do you have examples of method reference?
What is this? And how it looks like?

I found https://groups.google.com/g/Google-Web-Toolkit/c/8PXM54IwtDE/m/9naSp1CTYdcJ , so looks like @com.ecopy.gwt.client.Utils::messageBox(Ljava/lang/String;,Ljava/lang/ String;); is a jsni method reference. But I don't know why it can not be wrapped and how it can be used

It comes down to this: without more details or examples, we really have no idea what the author of the google style guide had in mind when they wrote it. If we want to make sure we are doing the right thing here, we should open an issue at https://github.com/google/styleguide?tab=readme-ov-file for clarification in this section.

@Zopsss , please create issue on Google style to clarify reason of such special allowance for long JSNI method reference as it is wrap-able.

@Zopsss your posts are very well written, thanks for that.

Thankk youuuu 😺✨

I am not sure what the lift required is to set up the GWT compiler, but at this point it might be a good idea to do this and see if such references can be line wrapped or not.

Yeahh I don't think many people uses GWT compiler. But it's better to ask the community before moving on

To be clear:

but at this point it might be a good idea to do this and see if such references can be line wrapped or not

This means that you might be able to get a definitive answer on the line wrapping without waiting for a reply in your posts.

The style guide makes it sound like it’s not possible to do this, but if it is, then this statement from the style guide is either outdated or plain incorrect.

This means that you might be able to get a definitive answer on the line wrapping without waiting for a reply in your posts.

Can you elaborate what do you mean by definitive answer?

The style guide makes it sound like it’s not possible to do this, but if it is, then this statement from the style guide is either outdated or plain incorrect.

The style guide is really old, I guess the documentation is not changed since it was created so it is outdated.

JSNI could not be detected right now, but might be possible after comments and javadoc support appear in Checkstyle

As you can see at the time google_checks.xml was created, Checkstyle did not had support for comments/javadocs.

Support for google java style guide was introduced in 2014. Here's the GSoC project link https://github.com/checkstyle/checkstyle/wiki/Checkstyle-GSoC-2014-Project-Ideas#project-name-checkstyle-configuration-for-googles-java-style-guide

Can you elaborate...

I am referring to the comment at #14501 (comment), if we are going to spend a bunch of time writing posts and waiting for replies, we can at least learn more about JSNI and see what the hubbub about line wrapping is.

ohh okhay, I would love to do more research on JSNI but my mid-term exams coming and I'm also working on other issues too, it is really difficult to get some time for doing the research stuff. So it would be really helpful if you or other maintainer can find the reason about why there is need to ignore JSNI method for LineLength module. I'm really sorry for asking maintainers to do this. @nrmancuso

romani · 2024-03-23T14:18:36Z

ok, @Zopsss , we need to move this PR from being stuck.

Please spend some time to get GWT example project to work in any IDE and test yourself if there is problem with line wrap.
https://www.gwtproject.org/
https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html#methods-fields
https://dzone.com/refcardz/gwt-style-configuration-and-js
https://livebook.manning.com/book/gwt-in-action-second-edition/chapter-11/84

please share images, log outputs of errors or no errors.

even we found that wrapping is not problem, it is success, as we can close issue on our issue tracker. And simply state to google style to remove this point about JSNI) or we will suggest to use your config only for people who carers, but looks like nobody care as nobody created issue to do such suppression. But your config will be in our issue as workaround.

Zopsss · 2024-03-23T20:46:44Z

ok, @Zopsss , we need to move this PR from being stuck.

Please spend some time to get GWT example project to work in any IDE and test yourself if there is problem with line wrap. https://www.gwtproject.org/ https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html#methods-fields https://dzone.com/refcardz/gwt-style-configuration-and-js https://livebook.manning.com/book/gwt-in-action-second-edition/chapter-11/84

please share images, log outputs of errors or no errors.

even we found that wrapping is not problem, it is success, as we can close issue on our issue tracker. And simply state to google style to remove this point about JSNI) or we will suggest to use your config only for people who carers, but looks like nobody care as nobody created issue to do such suppression. But your config will be in our issue as workaround.

yeah I'll do my research as soon as I get some time.

Zopsss · 2024-05-29T12:52:28Z

Back to this PR, so I was trying to setup GWT project to test if JSNI method references are wrap-able or not. But I had a doubt, how are we going to recognize the JSNI method reference?

Here is an example of jsni method reference:

this.@com.google.gwt.examples.JSNIExample::instanceFoo(Ljava/lang/String;)(s);

how can we recognize it using regex? I guess it is not possible?

The example was taken from official GWT documentation:

https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html

from Accessing Java fields from JavaScript's example section.

Plus IDEs now does not have support for GWT, it's really difficult to setup project locally and execute it. There are tutorials available on how to setup GWT project but they are mostly 7-8 years old, the setup process mentioned in documentation is also really old and outdated.

Should we close this issue? @romani

romani · 2024-06-02T14:35:28Z

https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html

(INFO: For new implementations use the future-proof JsInterop instead. JSNI will be removed with GWT 3.)

https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJsInterop.html

JsInterop is one of the core features of GWT 2.8. As the name suggests, JsInterop is a way of interoperating Java with JavaScript. It offers a better way of communication between the two using annotations instead of having to write JavaScript in your classes (using JSNI). More details about the annotations can be found in GWT javadoc: https://www.gwtproject.org/javadoc/latest/jsinterop/annotations/package-summary.html

romani · 2024-06-04T03:04:47Z

Ok.

long JSNI method reference

Still present at
https://google.github.io/styleguide/javaguide.html#s4.4-column-limit

I do see clear reason that it is deprecated, so no reason to add support for it.
But we have create special separate issue to explain all details of deprecation.
And our official position to explicitly exclude this request from coverage.
In scope of such new issue we simply update coverage table with note that jsni is not supported and not planned to be supported with link to new issue to read all details.

Please create new issue and let's close this PR and related issue as won't fix

Zopsss · 2024-06-04T15:21:38Z

Please create new issue and let's close this PR and related issue as won't fix

Done. New issue: #14938

romani requested changes Feb 18, 2024

View reviewed changes

Zopsss force-pushed the jsnimethod branch from 67b3ee9 to f0cd779 Compare February 18, 2024 18:48

Zopsss requested a review from romani February 18, 2024 18:50

nrmancuso assigned romani Feb 23, 2024

Zopsss force-pushed the jsnimethod branch from f0cd779 to fdc1d0a Compare February 23, 2024 18:24

Zopsss force-pushed the jsnimethod branch from fdc1d0a to 0dd911b Compare February 24, 2024 16:28

nrmancuso force-pushed the jsnimethod branch from 0dd911b to 2d80f72 Compare February 26, 2024 17:59

Zopsss force-pushed the jsnimethod branch from 2d80f72 to a400697 Compare February 28, 2024 16:32

Zopsss force-pushed the jsnimethod branch from 0b311ed to 1b4814d Compare February 29, 2024 04:10

nrmancuso requested changes Mar 3, 2024

View reviewed changes

src/xdocs/google_style.xml Outdated Show resolved Hide resolved

.../google/checkstyle/test/chapter4formatting/rule44columnlimit/InputLineLengthJsniMethods.java Outdated Show resolved Hide resolved

Issue checkstyle#14487: add support to ignore JSNI methods for LineLe…

40934dd

…ngth check in google config

Zopsss force-pushed the jsnimethod branch from 1b4814d to 40934dd Compare March 3, 2024 16:42

nrmancuso approved these changes Mar 3, 2024

View reviewed changes

romani approved these changes Mar 12, 2024

View reviewed changes

romani requested a review from rnveach March 12, 2024 04:41

romani assigned rnveach and unassigned romani Mar 12, 2024

rnveach reviewed Mar 13, 2024

View reviewed changes

rnveach assigned romani and nrmancuso Mar 13, 2024

nrmancuso removed their assignment Mar 13, 2024

Zopsss mentioned this pull request Mar 14, 2024

How the 4.4 Column Limit: 100 rule works for JSNI Method Reference? google/styleguide#808

Open

rnveach removed their assignment Mar 18, 2024

romani self-requested a review March 24, 2024 15:00

romani removed their assignment Mar 26, 2024

romani added the awaiting response label Mar 26, 2024

romani added blocked and removed awaiting response labels Jun 4, 2024

Zopsss mentioned this pull request Jun 4, 2024

Remove JSNI message from google style coverage page and explain the reason behind it #14938

Closed

Zopsss closed this Jun 4, 2024

Zopsss deleted the jsnimethod branch June 5, 2024 04:38

	<a href="https://webtoolkit.googleblog.com/2008/07/getting-to-really-know-gwt-part-1-jsni.html">
	JSNI</a>
	could not be detected right now, but might be possible after
	comments and javadoc support appear in Checkstyle.

Issue #14487: add support to ignore JSNI methods for LineLength check in google config #14501

Issue #14487: add support to ignore JSNI methods for LineLength check in google config #14501

Conversation

Zopsss commented Feb 18, 2024

Exceptions:

romani left a comment

Choose a reason for hiding this comment

Zopsss commented Feb 18, 2024

Zopsss commented Feb 18, 2024 • edited

Zopsss commented Feb 22, 2024

nrmancuso commented Feb 23, 2024

Zopsss commented Feb 23, 2024 • edited

nrmancuso commented Feb 23, 2024

Zopsss commented Feb 24, 2024

Zopsss commented Feb 26, 2024

Zopsss commented Feb 27, 2024 • edited

nrmancuso commented Feb 28, 2024

Zopsss commented Feb 28, 2024 • edited

Zopsss commented Feb 28, 2024

nrmancuso commented Feb 28, 2024

Zopsss commented Feb 29, 2024

github-actions bot commented Feb 29, 2024

Zopsss commented Feb 29, 2024

Zopsss commented Mar 2, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

Zopsss commented Mar 8, 2024

romani commented Mar 12, 2024

romani left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 12, 2024

romani commented Mar 12, 2024

Zopsss commented Mar 12, 2024

romani commented Mar 12, 2024

rnveach Mar 13, 2024 • edited

Choose a reason for hiding this comment

rnveach Mar 13, 2024 • edited

Choose a reason for hiding this comment

romani Mar 13, 2024

Choose a reason for hiding this comment

nrmancuso Mar 13, 2024

Choose a reason for hiding this comment

romani Mar 14, 2024

Choose a reason for hiding this comment

Zopsss Mar 14, 2024 • edited

Choose a reason for hiding this comment

nrmancuso Mar 14, 2024 • edited

Choose a reason for hiding this comment

Zopsss Mar 14, 2024 • edited

Choose a reason for hiding this comment

nrmancuso Mar 15, 2024

Choose a reason for hiding this comment

Zopsss Mar 16, 2024

Choose a reason for hiding this comment

romani commented Mar 23, 2024 • edited

Zopsss commented Mar 23, 2024

Zopsss commented May 29, 2024

romani commented Jun 2, 2024 • edited

romani commented Jun 4, 2024

Zopsss commented Jun 4, 2024

Zopsss commented Feb 18, 2024 •

edited

Zopsss commented Feb 23, 2024 •

edited

Zopsss commented Feb 27, 2024 •

edited

Zopsss commented Feb 28, 2024 •

edited

rnveach Mar 13, 2024 •

edited

rnveach Mar 13, 2024 •

edited

Zopsss Mar 14, 2024 •

edited

nrmancuso Mar 14, 2024 •

edited

Zopsss Mar 14, 2024 •

edited

romani commented Mar 23, 2024 •

edited

romani commented Jun 2, 2024 •

edited