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

Support Supression XML configuration in the Checkstyle.xml #65

Closed
isopov opened this Issue Nov 17, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@isopov
Copy link
Contributor

commented Nov 17, 2013

Moved from https://sourceforge.net/p/checkstyle/feature-requests/599/

When working with a large development team and certain checkstyle plugins (eclipse-cs for example), it is nice to have the checkstyle config in a single file. When needing to use the SuppressionFilter configuration, the relative path of the suppression xml file is not exactly intuitive, and may vary from development environment versus SVN hooks. Eclipse-cs even supports a remote location for the checkstyle config (which is handy for updating on the fly for the development team).

All this being said, It would be useful to have a single file to point to (checkstyle config). A solution may be to provide a simple SuppressionFilter implementation that allows configuration of folders to skip (a common ask, when skipping generated source folders, or test source files).

@romani

This comment has been minimized.

Copy link
Member

commented May 31, 2018

based on user story at #5756 (comment)

@rnveach , we are limited in checkstyle configuration to use attributes for modules.
But attributes can be multi-line, make ugly escaping on xml - https://www.freeformatter.com/xml-escape.html#ad-output in addition.
What is we will make following possible ?

<module name="SuppressionXpathFilter">
  <property name="content" value="
     &lt;suppress-xpath checks=&quot;CyclomaticComplexity&quot;
       files=&quot;FileOne.java,FileTwo.java&quot;
       query=&quot;//METHOD_DEF[@text=&apos;sayHelloWorld&apos;]&quot;/&gt;
     &lt;suppress-xpath checks=&quot;RequireThis&quot;
       query=&quot;/CLASS_DEF[@text=&apos;InputTest&apos;]
          //METHOD_DEF[@text=&apos;changeAge&apos;]//ASSIGN[@text=&apos;age&apos;]/IDENT&quot;/&gt;
   "/>
  <property name="optional" value="false"/>
</module>

un-escaped xml is:

  <suppress-xpath checks="CyclomaticComplexity"
    files="FileOne.java,FileTwo.java"
    query="//METHOD_DEF[@text='sayHelloWorld']"/>
  <suppress-xpath checks="RequireThis"
    query="/CLASS_DEF[@text='InputTest']
          //METHOD_DEF[@text='changeAge']//ASSIGN[@text='age']/IDENT"/>
@rnveach

This comment has been minimized.

Copy link
Member

commented May 31, 2018

What if we will make following possible ?

  1. Multi-lining doesn't always work out. In suppressions you need an extra | between lines and it had to be regexp. In violation messages, it causes the message to print the line control which messed up the audit display.
  2. Even if there is a tool to do it for us, it isn't going to be easy escaping a huge chunk of XML, and it is going to be harder to read it or even modify it in the future. I can't see the users liking it this way.

Since this is an issue in an external tool, why doesn't the tool address it?
If we are forced to accommodate this, I would rather merge all the DTDs together in a separate, single configuration file.

<configuration>

  <module name="Checker">
    <module name="TreeWalker">
      <module name="EmptyBlock"/>
    </module>
  </module>

  <suppressions>
    <suppress checks="EmptyBlock" files=".*[\\/]src[\\/]test[\\/]"/>
  </suppressions>

  <import-control pkg="com.puppycrawl.tools.checkstyle">
    <disallow pkg="junit.framework" />

    <allow pkg=".*" regex="true" />
  </import-control>

</configuration>
@romani

This comment has been minimized.

Copy link
Member

commented May 31, 2018

I would rather merge all the DTDs together in a separate, single configuration file.

Do not forget about multiple instances of the same module.
In this case all file configuration should be inlined under module tag and be by special new tag "configuration" for example, and under such tag there should be whole freedom of custom tags. Not clear what format should be if module have few extrnal files.
There are a lot of nuances and non of us had time to think about them thoroughly.

Multi-lining doesn't always work out.

Please share example.

it isn't going to be easy escaping a huge chunk of XML

Yes, it is workaround. Better than nothing.
We do not force users to use this property, but when they need it they can decide on benefits vs cons.
Some people work with almost no suppression at all, so small amount in ugly form might be ok, as I should not be changed easily.
Users that use suppression on daily basis, most likely will not use it, but they are ok to play with config in jar and reconciling IDEs, a lot of companies use one IDE only.

why doesn't the tool address it?

they will not care about us at all. For maven we just another plugin from thousands, and you know state of maven checkstyle plugin. Eclipse and IDEA support external files , but support in build systems (maven, gradle) is a problem.

@romani

This comment has been minimized.

Copy link
Member

commented May 31, 2018

@jodastephen, can you share some opinion as user of checkstyle ?

@jodastephen

This comment has been minimized.

Copy link
Contributor

commented May 31, 2018

This comment seems like a good approach to me. ie. putting each file in one large XML file by merging the DTDs. Using long attributes would not be pleasant.

@rnveach

This comment has been minimized.

Copy link
Member

commented May 31, 2018

Do not forget about multiple instances of the same module.

This has no bearing on the full config file layout. My snippet was just a simple example.
Each piece of the full config file should handle it pieces the exact same as they do now. The only difference is it is 3 files in 1. Multiple or single modules, suppressions, import controls don't matter. 1 full configuration file should act the same as copying and pasting the pieces into their own, separate files.

Not clear what format should be if module have few extrnal files.

For external files, we would need some new coding for the property value that says to not look for external file, but find it in full configuration file. External file properties currently supports file system, URL, and classpath. We would just need a one word statement like configuration for it to know to look at the full configuration file.

Multi-lining doesn't always work out.

Please share example.

My post gave examples. An example of issue with suppressions can be seen at https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml#L23-L24 and in the PR where you stated the reason for doing this. There is no example of new lines in messages as I suppressed these for the 100 character line length once I found the issue. See https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml#L8-L9 . The same was not done for our configuration file, so you can see the results when a violation is produced for them.

they will not care about us at all. For maven we just another plugin from thousands, and you know state of maven checkstyle plugin.
Better than nothing.

And this is why it isn't our problem. If tools don't want to support it, then users should find another tool that supports their needs. I have already stated we should break away from these tools in our own repo to force the tools to either be updated or force new tools to be created in their place by people who will support it. Maven-checkstyle didn't release a new version until I created the issue threatening breaking compatibility with them.

I don't support inlining an XML file inside an XML property, I gave my reasons. I am ok to support the idea of the full configuration file but I advise against using it as it is too much in 1 file, just like our counts and NCSS checks warn us about Java code.

@romani

This comment has been minimized.

Copy link
Member

commented May 31, 2018

inlining of configuration looks good agree, no arguing but this is big project, looks on what we have now in our project, it is kind of not-doable in near future, backlog is already too much, bunch of big projects like this already stuck and end-up in nothing.

Another idea - new filter with all properties of external file tag - discussion for new filter is at #5879

@romani

This comment has been minimized.

Copy link
Member

commented Jun 2, 2018

I am removed unrelated to this issue comments, to keep issue clean for off-topic discussions.
New filter will be some workaround for some users.

This issue still under discussion , if somebody interested, please continue to share ideas for design.

@checkstyle checkstyle deleted a comment from rnveach Jun 2, 2018

@checkstyle checkstyle deleted a comment from jodastephen Jun 2, 2018

@romani

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I think users will be satisfied with #6402 instead.

I am closing this issue as there is no activity in it for a while.

NOTE for users: if your use case can not be covered by "Single" filters, please share them here we can reopen issue or ....

@romani romani closed this Apr 13, 2019

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