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

[Java] Checkstyle CLI while generating xpath suppressions provides extraneous stdout output. #6974

Closed
Fernal73 opened this issue Aug 12, 2019 · 14 comments

Comments

@Fernal73
Copy link

commented Aug 12, 2019

https://checkstyle.org/cmdline.html#Command_line_usage

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
  "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
  "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="haltOnException" value="false"/>
  <module name="TreeWalker">
    <module name="TypeName" />
  </module>
</module>

$ cat Test.java 
class T_est {}
rivanov@p5510:/var/tmp$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] /var/tmp/Test.java:1:7: Name 'T_est' must match pattern '^[A-Z][a-zA-Z0-9]*$'. [TypeName]
Audit done.
Checkstyle ends with 1 errors.

$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -g -c config.xml Test.java 
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suppressions PUBLIC
    "-//Checkstyle//DTD SuppressionXpathFilter Experimental Configuration 1.2//EN"
    "https://checkstyle.org/dtds/suppressions_1_2_xpath_experimental.dtd">
<suppressions>
<suppress-xpath
       files="Test.java"
       checks="TypeNameCheck"
       query="/CLASS_DEF/IDENT[@text='T_est']"/>
</suppressions>
Checkstyle ends with 1 errors.

$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -g -o suppresion.xml -c config.xml Test.java 
Checkstyle ends with 1 errors.

$ cat suppresion.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suppressions PUBLIC
    "-//Checkstyle//DTD SuppressionXpathFilter Experimental Configuration 1.2//EN"
    "https://checkstyle.org/dtds/suppressions_1_2_xpath_experimental.dtd">
<suppressions>
<suppress-xpath
       files="Test.java"
       checks="TypeNameCheck"
       query="/CLASS_DEF/IDENT[@text='T_est']"/>
</suppressions>

$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -o out.txt -c config.xml Test.java 
Checkstyle ends with 1 errors.

$ cat out.txt 
Starting audit...
[ERROR] /var/tmp/Test.java:1:7: Name 'T_est' must match pattern '^[A-Z][a-zA-Z0-9]*$'. [TypeName]
Audit done.

$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -o out.xml -f xml -c config.xml Test.java 
Checkstyle ends with 1 errors.

$ cat out.xml 
<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="8.24-SNAPSHOT">
<file name="/var/tmp/Test.java">
<error line="1" column="7" severity="error" message="Name &apos;T_est&apos; must match pattern &apos;^[A-Z][a-zA-Z0-9]*$&apos;." source="com.puppycrawl.tools.checkstyle.checks.naming.TypeNameCheck"/>
</file>
</checkstyle>

$ cat Test.java 
class Test {}

$ java -jar checkstyle-8.24-SNAPSHOT-all.jar -c config.xml Test.java 
Starting audit...
Audit done.

OUTPUT

"Checkstyle ends with n errors" is displayed if violations exist. This is output to stdout instead of stderr, unlike the log messages.
This occurs whether the -o option is in use or not. No xml document is generated if violations under TreeWalker module are not detected.


Describe what you expect in detail.

The informative message should not be output as stdout message when the -g option is used. My understanding is that the -g option is used for generating the suppression document specifically. No other stdout output should be provided. Log and other messages must be stderr output. This way, the stdout xml document can be captured cleanly and saved in a file, if needed.


Spun off from #6934

@Fernal73

This comment has been minimized.

Copy link
Author

commented Aug 12, 2019

Is it possible to rename the above issue? If yes, can the [java] prefix be removed? Thanks

@romani

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@Fernal73 , please always report issues to us in CLI form with examples of behavior. Your issues are very hard to read, it take a while to get point. Please review how I updated issue, and please follow this approach.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

So ...
Checkstyle ends with X errors. is shown only when violation with ERROR severity is present (if severity is warning, no such violation). This sentence is not part of validation output of plain format and not a xml output (obvious).
We can print this sentence in stderr output only, looks like sterr reasonable place for it.

@rnveach and @pbludov , does it make sense to you ? approved ?

FYI: such printing in stdout was NOT done on good purpose, it was introduced during big refactoring of Main.java a while a ago.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@strkkk , please share your ideas too.

@strkkk

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

In my opinion this line should not be in output for -g mode, because it says it generates a file, but in fact it does not.
For example, this wont produce valid xml in 8.23
java -jar checkstyle.jar -c conf -g > suppr.xml because of this extra line, you need to grep output first or something.

Upd: Also, this message looks a bit strange because checkstyle didn't end with errors, because it works in another mode.

@Fernal73

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@romani


java -jar checkstyle-8.24-SNAPSHOT-all.jar -g -o suppresion.xml -c config.xml Test.java 
Checkstyle ends with 1 errors.

$ cat suppresion.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suppressions PUBLIC
    "-//Checkstyle//DTD SuppressionXpathFilter Experimental Configuration 1.2//EN"
    "https://checkstyle.org/dtds/suppressions_1_2_xpath_experimental.dtd">
<suppressions>
<suppress-xpath
       files="Test.java"
       checks="TypeNameCheck"
       query="/CLASS_DEF/IDENT[@text='T_est']"/>
</suppressions>

Is this the new behaviour? That wasn't happening earlier. Then the suppression file is as expected with no additional output invalidating the xml document. You might wish to update the documentation to reflect this new behaviour with option -g.

@strkkk Grepping out the extra line is not a good idea.

The objection raised was to the additional line. I saw no need to create examples since the generated output is irrelevant in this case.

It may be difficult for a newbie to comprehend the gist of my report without actually seeing the other outputs.

@romani Thanks for the example output and updating the bug for clarity.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

For example, this wont produce valid xml in 8.23
java -jar checkstyle.jar -c conf -g suppr.xml because of this extra line, you need to grep output first or something.

The same is for when you define -f xml. To get good report user need to use -o.
I do not remember why I added this static line to the end. It might be reasonable to move it to plain report content and even to xml as attribute of top tag.
But probably it is better to remove such line, as it prints only errors, but report can have different severities also. If somebody needs such static we can print severity based in reports.

Let's wait for others response.I am in favor to remove such line at all.

@rnveach

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I am fine with printing to stderr. As long as it is not in the output XML file, is fine with me.
If we want to add it to the XML as it's own tag, that is also fine.

@Fernal73

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

If added to the suppressions xml, it must not be a mandatory element but an optional one since it adds nothing to the usage of the suppressions document.

My usecase involves generating an initial suppressions-xpath.xml via the above CLI. Then, additional suppressions are added via cut and paste as the code base grows with only 'unavoidable' violations suppressed. 'Unavoidable' is in quotes as the definition is subjective.

Also, with this fixed, we're only a step away from providing the user the option of saving the xml to a user-specified file and/or appending it to an existing one.

@strkkk

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

to xml as attribute of top tag

I really doubt someone would need it, it is only informational thing and it is equal to number of suppressions in generated file.

@Fernal73

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

@strkkk Not always. Suppression xml lists only violations under TreeWalker module. However, that additional information is not needed in the suppressions document. With an error file now generated as well with the -o option, this becomes redundant as part of stdout. I'm fine with stderr. As long as it doesn't pollute the suppressions xml. Ideally, we ought to be sending the suppressions xml to a file as well.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

If added to the suppressions xml

I didn't mean suppression xml. I meant violations xml report.

We just need to remove this static line from output.

@Fernal73

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

@romani
Of course, it is. How asinine of me!

@rnveach

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Fix was merged

@rnveach rnveach closed this Aug 16, 2019

@rnveach rnveach added this to the 8.24 milestone Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.