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 #7763: Update AbstractChecks to log DetailAST - TrailingComment #8711

Merged
merged 1 commit into from Dec 2, 2020

Conversation

HuGanghui
Copy link
Contributor

@HuGanghui HuGanghui commented Aug 18, 2020

Issue #7763: Update AbstractChecks to log DetailAST - TrailingComment

from Roman: it is not implementation that I would like to have in this Check ideally, but let move refactoring to new issue.
#9009
#9010

Diff Regression projects: https://gist.githubusercontent.com/HuGanghui/709e0a8266eb1656488051b4cfc9161f/raw/5731ab3b42c995e5823e02a34a00c25241bcec1d/projects-to-test-on.properties

Diff Regression config: https://gist.githubusercontent.com/HuGanghui/d3b892a11fa52287ec1723246f12718f/raw/9576b09b677ea0cf838aa3ae451519597efd46ca/TrailingComment.xml

@HuGanghui
Copy link
Contributor Author

Github, generate report

@romani
Copy link
Member

romani commented Aug 21, 2020

@HuGanghui, please take a look at other PR for example of report generation, you my to provide links to configs
Instructions https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#report-generation

@HuGanghui
Copy link
Contributor Author

Github, generate report

@github-actions
Copy link
Contributor

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/220867385

@HuGanghui
Copy link
Contributor Author

Github, generate report

@github-actions
Copy link
Contributor

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/220869516

@HuGanghui
Copy link
Contributor Author

Github, generate report

@github-actions
Copy link
Contributor

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/220871017

@HuGanghui
Copy link
Contributor Author

HuGanghui commented Aug 23, 2020

@romani

Diff Regression projects: https://gist.githubusercontent.com/nmancus1/e3988f8092d919b5cbe70f5359c4d9e8/raw/a0f6f5860f8025c19868061dee6e0e40960b992f/projects-to-test-on.properties

at the beginning, I try to use Diff Regression projects link that in #8752 (comment), and then github-actions failed, so I use my own Diff Regression projects link https://gist.githubusercontent.com/HuGanghui/709e0a8266eb1656488051b4cfc9161f/raw/5731ab3b42c995e5823e02a34a00c25241bcec1d/projects-to-test-on.properties but it still not work now. please give me some help.

@HuGanghui
Copy link
Contributor Author

CI error is that I introduce new check, but I am not:

Diff is detected.
283PR Description grepped:
284You introduce new Check
285--- .ci-temp/verify-no-exception-configs/web.txt 2020-08-23 17:15:34.837125664 +0000
286+++ .ci-temp/verify-no-exception-configs/file.txt 2020-08-23 17:15:34.845125665 +0000
287@@ -139,7 +139,6 @@
288 RegexpOnFilename
289 RegexpSingleline
290 RegexpSinglelineJava
291-RequireEmptyLineBeforeBlockTagGroup
292 RequireThis
293 ReturnCount
294 RightCurly
295Please create PR to repository https://github.com/checkstyle/contribution
296and add your new Check
297 to file checkstyle-tester/checks-nonjavadoc-error.xml
298or to file checkstyle-tester/checks-only-javadoc-error.xml
299PR for contribution repository will be merged right after this PR.

@romani
Copy link
Member

romani commented Nov 19, 2020

Github, generate report

@github-actions
Copy link
Contributor

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/372541131

@romani
Copy link
Member

romani commented Nov 19, 2020

Github, generate report

@romani
Copy link
Member

romani commented Nov 19, 2020

@timurt ,

do you see why I can not match comment by content ? even != does not work

$ cat Test.java 
package org.checkstyle.suppressionxpathfilter.trailingcomment;

public class SuppressionXpathRegressionTrailingComment1 {
   int i; // do
}

$ java -jar checkstyle-8.36.2-all.jar -b \
 "/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionTrailingComment1']]/OBJBLOCK/SINGLE_LINE_COMMENT/COMMENT_CONTENT" Test.java 
CLASS_DEF -> CLASS_DEF [3:0]
`--OBJBLOCK -> OBJBLOCK [3:56]
   |--SINGLE_LINE_COMMENT -> // [4:11]
   |   `--COMMENT_CONTENT ->  do\n [4:13]

$ java -jar checkstyle-8.36.2-all.jar -b \
 "/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionTrailingComment1']]/OBJBLOCK/SINGLE_LINE_COMMENT/COMMENT_CONTENT[@text=' do\n']" Test.java 

$ java -jar checkstyle-8.36.2-all.jar -b \
 "/CLASS_DEF[./IDENT[@text='SuppressionXpathRegressionTrailingComment1']]/OBJBLOCK/SINGLE_LINE_COMMENT/COMMENT_CONTENT[@text!=' do\n']" Test.java 

how to escape ' in xpath - #6893 , by ''(extra singe quote symbol).

we need to find a way to write suppression xpath queries for cases from
http://roman-ivanov.blogspot.com/2013/10/trailing-comment-check-false-positives.html

@romani
Copy link
Member

romani commented Nov 20, 2020

$ cat Test.java 
public class Test {
    int i; // warn
    int[] arr = {
      40, // ok magic reason
      47, // ok magic reason
    };
}

$ java -jar checkstyle-8.36.2-all.jar -b "//ARRAY_INIT//SINGLE_LINE_COMMENT/COMMENT_CONTENT" Test.java 
CLASS_DEF -> CLASS_DEF [1:0]
`--OBJBLOCK -> OBJBLOCK [1:18]
    |--VARIABLE_DEF -> VARIABLE_DEF [3:7]
    |   |--ASSIGN -> = [3:14]
    |   |   `--ARRAY_INIT -> { [3:16]
    |   |       |--SINGLE_LINE_COMMENT -> // [5:10]
    |   |       |   `--COMMENT_CONTENT ->  ok magic reason\n [5:12]
---------
CLASS_DEF -> CLASS_DEF [1:0]
`--OBJBLOCK -> OBJBLOCK [1:18]
    |--VARIABLE_DEF -> VARIABLE_DEF [3:7]
    |   |--ASSIGN -> = [3:14]
    |   |   `--ARRAY_INIT -> { [3:16]
    |   |       |--EXPR -> EXPR [5:6]
    |   |       |   |--SINGLE_LINE_COMMENT -> // [4:10]
    |   |       |   |   `--COMMENT_CONTENT ->  ok magic reason\n [4:12]

@timurt
Copy link
Contributor

timurt commented Nov 21, 2020

@romani

Generally, we do not support @text attribute for all token types, only for certain nodes that are described here.

I added TokenTypes.COMMENT_CONTENT token type to the list that I mentioned and assembled project to jar. After that I was able to query this COMMENT_CONTENT node by attribute value.

ex:

$ java -jar checkstyle-8.38-SNAPSHOT-all.jar -J Test.java -b "//COMMENT_CONTENT[starts-with(@text, ' do')]"

Here I used starts-with only because attribute contained escape character and I couldn't query it. I suppose we need to create a separate issue, if we want to handle COMMENT_CONTENT token type

@romani
Copy link
Member

romani commented Nov 22, 2020

current implementation works like:

$ cat Test.java 
public class Test {
    int p; /* warn
              second line */
    int i; // warn
    int[] arr = {
      40, // ok magic reason
      47, // ok magic reason
    };
}

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
      <module name="TrailingComment" />

      <module name="SuppressionXpathSingleFilter">
        <property name="checks" value="TrailingComment"/>
        <property name="query" value="//ARRAY_INIT//SINGLE_LINE_COMMENT"/>
      </module>

    </module>
</module>

$ java -jar checkstyle-8.38-SNAPSHOT-all.jar -c config.xml  Test.java
Starting audit...
[ERROR] /var/tmp/Test.java:2:12: Don't use trailing comments. [TrailingComment]
[ERROR] /var/tmp/Test.java:4:12: Don't use trailing comments. [TrailingComment]
Audit done.
Checkstyle ends with 2 errors.

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.

I am ok to merge

@romani
Copy link
Member

romani commented Nov 25, 2020

Github, generate report

@romani
Copy link
Member

romani commented Nov 25, 2020

I just rebased (to fix wercker) after requesting report, code is the same.

@romani
Copy link
Member

romani commented Nov 30, 2020

Github, rebase

@romani romani assigned romani and unassigned strkkk Nov 30, 2020
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.

None yet

4 participants