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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions config/jsoref-spellchecker/whitelist.words
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,6 @@ GMetrics
gnupg
google
googleapis
googleblog
googlecloudplatform
googleecommon
googlegroups
Expand Down Expand Up @@ -1417,7 +1416,6 @@ webjar
Weblogic
Webp
website
webtoolkit
Wellformedness
wget
wherejavadocrequired
Expand Down
19 changes: 19 additions & 0 deletions .../java/com/google/checkstyle/test/chapter4formatting/rule44columnlimit/LineLengthTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.Test;

import com.google.checkstyle.test.base.AbstractGoogleModuleTestSupport;
import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
import com.puppycrawl.tools.checkstyle.api.Configuration;
import com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck;

Expand All @@ -46,4 +47,22 @@ public void testLineLength() throws Exception {
verify(checkConfig, filePath, expected, warnList);
}

@Test
public void testLineLengthJsniMethods() throws Exception {
final String[] expected = {
"14: " + getCheckMessage(LineLengthCheck.class, "maxLineLen", 100, 165),
};

final Configuration lineLengthConfig = getModuleConfig("LineLength");
final DefaultConfiguration rootConfig = createRootConfig(lineLengthConfig);

final Configuration filterConfig = getModuleConfig("SuppressWithPlainTextCommentFilter");
rootConfig.addChild(filterConfig);

final String filePath = getPath("InputLineLengthJsniMethods.java");
Zopsss marked this conversation as resolved.
Show resolved Hide resolved

final Integer[] warnList = getLinesWithWarn(filePath);
verify(rootConfig, filePath, expected, warnList);
}

}
16 changes: 16 additions & 0 deletions ...ogle/checkstyle/test/chapter4formatting/rule44columnlimit/InputLineLengthJsniMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.google.checkstyle.test.chapter4formatting.rule44columnlimit;

public class InputLineLengthJsniMethods {

// JSNI method
public static native void alertMessage(String msg) /*-{
$wnd.alert(msg);
console.log('my very long message ....................................................................................................................');
}-*/;

// just a comment, no JSNI delimiters
public static native void alertMessage2(String msg) /*
$wnd.alert(msg);
console.log('my very long message ....................................................................................................................'); // warn
*/;
}
8 changes: 8 additions & 0 deletions src/main/resources/google_checks.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@
<property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/>
</module>

<!-- To ignore JSNI Methods for LineLength check -->
<!-- See https://google.github.io/styleguide/javaguide.html#s4.4-column-limit -->
<module name="SuppressWithPlainTextCommentFilter">
<property name="offCommentFormat" value="\/\*-\{"/>
<property name="onCommentFormat" value="\}-\*\/"/>
Copy link
Member

@rnveach rnveach Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@rnveach rnveach Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Zopsss Zopsss Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member

@nrmancuso nrmancuso Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Zopsss Zopsss Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<property name="checkFormat" value="LineLength"/>
</module>

<module name="TreeWalker">
<module name="OuterTypeFilename"/>
<module name="IllegalTokenText">
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,6 +1665,7 @@ public void testAllStyleRules() throws Exception {
styleChecks.remove("SuppressionCommentFilter");
styleChecks.remove("SuppressWarningsFilter");
styleChecks.remove("SuppressWarningsHolder");
styleChecks.remove("SuppressWithPlainTextCommentFilter");

assertWithMessage(
fileName + " requires the following check(s) to appear: " + styleChecks)
Expand Down
4 changes: 4 additions & 0 deletions src/xdocs/filters/suppresswithplaintextcommentfilter.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,10 @@ public class Example9 {
</subsection>
<subsection name="Example of Usage" id="Example_of_Usage">
<ul>
<li>
<a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWithPlainTextCommentFilter">
Google Style</a>
</li>
<li>
<a href="https://github.com/search?q=path%3Aconfig%20path%3A**%2Fcheckstyle-checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWithPlainTextCommentFilter">
Checkstyle Style</a>
Expand Down
4 changes: 4 additions & 0 deletions src/xdocs/filters/suppresswithplaintextcommentfilter.xml.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@
</subsection>
<subsection name="Example of Usage" id="Example_of_Usage">
<ul>
<li>
<a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWithPlainTextCommentFilter">
Google Style</a>
</li>
<li>
<a href="https://github.com/search?q=path%3Aconfig%20path%3A**%2Fcheckstyle-checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWithPlainTextCommentFilter">
Checkstyle Style</a>
Expand Down
12 changes: 2 additions & 10 deletions src/xdocs/google_style.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,18 +780,10 @@
<td>
<span class="wrapper inline">
<img
src="images/ok_blue.png"
src="images/ok_green.png"
alt="" />
</span>
<a href="checks/sizes/linelength.html#LineLength">LineLength</a>
<br />
We can detect URL with protocol type as http://, https:// etc.
<br />
<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.
<br />
</td>
<td>
<a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+LineLength">
Expand Down Expand Up @@ -2362,7 +2354,7 @@
<a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressionCommentFilter">
SuppressionCommentFilter</a>,
<a href="https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fgoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+SuppressWarningsFilter">
SuppressWarningsFilter</a>,
SuppressWarningsFilter</a>.
</p>
</subsection>
</section>
Expand Down