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

SuppressWarnings flexibility #120

Closed
ruby0x1 opened this issue Mar 7, 2016 · 34 comments
Closed

SuppressWarnings flexibility #120

ruby0x1 opened this issue Mar 7, 2016 · 34 comments

Comments

@ruby0x1
Copy link

ruby0x1 commented Mar 7, 2016

I've been looking at using this lib for a while, but there are some important things preventing me from using it in an automated or realistic project context.

The most important is the control over suppression is not sufficient, and has some concerns.
Two of them are just about what you can suppress and how:

  • Suppressing whole packages from checking
  • Suppressing “all” checks, instead of explicitly stating them one by one

The next issue is a bigger for me, which is that in order to suppress anything, modifying the actual code is required. This is an instant blocker, because decorating code with arbitrary third party/external meta information is not an option. Not just because this makes a mess, but this becomes impossible if you need to suppress things from third party or vendor code that exists within your codebase.

My ideas to solve this would be to provide more specific suppression control rules, like:

  • some.pack
  • some.pack.Class
  • some.pack.Class:method

This gives you enough control to configure the project. It's possible to add :method:lines or similar but I think since code changes, line specific suppression seems impractical.

And secondly, make an exceptions node or similar inside the rules.json which can contain a predefined list of the above suppression markers, i.e

{
  "exceptions": {
    "some.pack" : "all",
    "some.pack.Class:someMethod" : "Dynamic, LineLength"
  }, 
  ...
}

This allows a user to keep all their exceptions to the project rules localized, rather than spread out across the source code, doesn't require modifying the code, and to me, encourages more specific and conscious additions to the exceptions with a cleaner lineage as to where and why they're added.

Note this shouldn't remove the meta option, just complement it and give alternative options and flexibility. I would however align the meta tag closer with Haxe style than the Java style personally :)

Any thoughts?

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

I really like the idea of specifying exceptions in JSON which is much cleaner.

@adireddy adireddy self-assigned this Mar 7, 2016
@Gama11
Copy link
Member

Gama11 commented Mar 7, 2016

I like the suggestion too - I really dislike modifying the source code to suppress warnings. I've thought about package-wide suppressions as well (there are a few cases in checkstyle itself where this would be useful - for instance suppressing certain checks only in the tests).

The .json field should maybe be named suppressions instead of exceptions for consistency with the metadata.

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

I prefer keeping exceptions or suppressions in the same rules JSON file instead of another JSON.

@Gama11
Copy link
Member

Gama11 commented Mar 7, 2016

I don't think anybody suggested otherwise?

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

The .json field I misread it :)

@AlexHaxe
Copy link
Member

AlexHaxe commented Mar 7, 2016

I vote against putting them into the same JSON :)
I use one config file for all my projects, so cluttering it with exceptions from many different projects doesn't seem like a good idea.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

Mainly the language of rules and exceptions to those rules makes more sense to me than suppressions but it doesn't matter.

That's a good point @AlexHaxe, it might more sense like -c rules.json -s exceptions.json instead (or similar).

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

In fact, having both might not be a problem either. In this case I don't see increased flexibility or options as a downside or undue complexity (it can just a matter of combining rules from both, which is simple for json and haxe).

i.e a cli option for the exceptions file separate, as well as the ones within the rules file ("default exceptions" if you want to word them that way)

@Gama11
Copy link
Member

Gama11 commented Mar 7, 2016

-s is already used for source paths.

We could allow mulitple -c arguments like we already do with -s.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

yea -s was just randomly picked for "suppressions", not a proper suggestion 🎱

That could work too, if all -c files are parsed and the suppressions node merged along with the rules, that would be a single implementation that covers both use cases.

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

Thoughts on the following format? @underscorediscovery @AlexHaxe @Gama11

{
    "exclude": [
        {
            "packages": [
                "test.checks",
                "reports"
            ],
            "classes": [
                "utils.TestUtils"
            ],
            "methods": [
                "utils.BrowserUtils:getPixelRatio"
            ],
            "checks": [
                "all"
            ]
        },
        {
            "classes": [
                "utils.DeviceUtils"
            ],
            "checks": [
                "Dynamic",
                "LineLength"
            ]
        }
    ]
}

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

Why so verbose when the same information can be expressed in one key:value per rule?

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

Something like this then?

{
    "exclude": {
        "test.checks" : "all",
        "reports" : "all",
        "utils.TestUtils": "all",
        "utils.BrowserUtils:getPixelRatio": "all",
        "utils.DeviceUtils": "Dynamic, LineLength"
    }
}

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

That is much cleaner to me, and definitely better.
It fits well with haxe ways of referencing a type, and is a simple and easy to understand system:

"suppression context" : "suppressed rules"

This allows more contexts to exist without so many json nodes per context, and makes it simpler to migrate/update/change the contexts as needed over time.

@adireddy
Copy link
Member

adireddy commented Mar 7, 2016

How abt this? Grouping them by checks. I like the above format but afraid of scattered excludes if there are too many.

{
    "exclude": {
        "all": ["test.checks", "reports", "utils.TestUtils", "utils.BrowserUtils:getPixelRatio"],
        "Dynamic": ["utils.DeviceUtils"],
        "LineLength": ["utils.DeviceUtils"]
    }
}

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

I think both have trade offs, in the above you have duplicate classes, in the alternate you have duplicate rules. I prefer the way it reads with context:rules personally but it doesn't matter in practice as long as it's not overly verbose, and solves the needs.

@Gama11
Copy link
Member

Gama11 commented Mar 7, 2016

Grouping by package / type / field seems more natural to me.

I'm not sure about referencing fields with : - ("utils.BrowserUtils:getPixelRatio"), I haven't seen that syntax anywhere.

There's the C++-like option of ::field, or just a regular dot (.field), but that always reads like "static field" to me. I personally use .field for static fields and #field for instance fields.

Btw, I think the value should be an array (["Dynamic", "LineLength"] instead of "Dynamic, LineLength").

Is there also a use case for the inverse? Only activate certain checks on certain packages / types / fields.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 7, 2016

Yea I think a filtered include is also a good idea, since I actually have (on first run) over 6000 messages just in one package to deal with - I would love to work by check, and I would love to work by package, type, or even function.

The other option for field is class@field (which can include members variables right?). I would look at whatever haxe uses most consistently and be consistent with the language first.

@Gama11
Copy link
Member

Gama11 commented Mar 8, 2016

You can already work by check by starting out with default-config.json, where all checks have "severity": "IGNORE", and enable them step by step.

I don't think Haxe really has a syntax for accessing instance fields through the class name?

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 8, 2016

Mostly referring to the way haxe refers to things, which may actually be target specific - I'm not certain off hand.

screen 2016-03-07 at 8 56 26 pm

I know I can set a bunch to ignore but I was agreeing that the inverse of opt in is useful for filtering when you want tackle a specific thing in isolation.

@AlexHaxe
Copy link
Member

AlexHaxe commented Mar 8, 2016

When you have over 6000 checkstyle messages, that can be a bit overwhelming. My guess is a large portion of those messages will go away, once you start configuring checkstyle to use your code style instead of the default.

If you are on a Unix machine (your screenshot uses / so I guess you are), then you can run checkstyle in a command line window and use grep to filter for a particular message that you want to fix. e.g. haxelib run checkstyle -s snow | grep Trailing - which will show you only the files that have trailing whitespace issues (assuming checkstyle.json exists in your project root, otherwise add -c myconfig.json).

Using grep you can work through a huge number of issues in a quick way, because you are looking at only one problem at a time. And once you fixed the first occurrence, the rest should be faily similar (unless it is a CyclomaticComplexity, MethodLength, ParameterNumber or TODOComment issue, those might take a bit longer to fully fix, so leave them for the end).

@adireddy adireddy mentioned this issue Mar 9, 2016
@adireddy
Copy link
Member

adireddy commented Mar 9, 2016

@underscorediscovery implemented package level and class level exclude functionality in dev branch.

You can look at the config used for this project here.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 9, 2016

For interest sake I converted it to the alternate that I and @Gama11 mentioned, and sorted them by type just so we can compare.

{
    "checks" : ["EmptyLines", "MultipleStringLiterals"],
    "token" : ["MultipleStringLiterals"],

    "checks.coding.MultipleVariableDeclarationsCheckTest" : ["MultipleVariableDeclarations"],
    "checks.naming.MemberNameCheckTest" : ["TrailingWhitespace"],
    "checks.size.LineLengthCheckTest" : ["LineLength"],
    "checks.whitespace.IndentationCharacterCheckTest":["TabForAligning", "IndentationCharacter"],
    "checks.whitespace.SpacingCheckTest" : ["TrailingWhitespace"],
    "checks.whitespace.TabForAligningCheckTest" : ["TabForAligning"],
    "checks.whitespace.TrailingWhitespaceCheckTest" : ["TrailingWhitespace"],

    "checkstyle.Main" : "Dynamic",
    "checkstyle.Checker" : "Dynamic",
    "checkstyle.ChecksInfo" : "Dynamic",

    "checkstyle.token.TokenTreeBuilder" : ["CyclomaticComplexity", "MethodLength"],
    "checkstyle.reporter.ProgressReporter" : ["MagicNumber"],
    "checkstyle.utils.ExprUtils":["CyclomaticComplexity"],
    "checkstyle.utils.ComplexTypeUtils":["CyclomaticComplexity"],

    "checkstyle.checks.Check":["CyclomaticComplexity"],
    "checkstyle.checks.CyclomaticComplexityCheck" : ["CyclomaticComplexity", "LeftCurly", "RightCurly"],
    "checkstyle.checks.block.LeftCurlyCheck":["CyclomaticComplexity"],
    "checkstyle.checks.block.RightCurlyCheck":["CyclomaticComplexity"],
    "checkstyle.checks.coding.MultipleVariableDeclarationsCheck" : ["MultipleVariableDeclarations"],
    "checkstyle.checks.naming.MethodNameCheck":["CyclomaticComplexity"],
    "checkstyle.checks.type.ReturnCheck":["CyclomaticComplexity"],
    "checkstyle.checks.whitespace.OperatorWrapCheck":["CyclomaticComplexity"],
    "checkstyle.checks.whitespace.WhitespaceAfterCheck" : ["CyclomaticComplexity", "MethodLength"],
    "checkstyle.checks.whitespace.WhitespaceAroundCheck" : ["CyclomaticComplexity", "MethodLength"]
}

@Gama11
Copy link
Member

Gama11 commented Mar 11, 2016

It's possible to run checkstyle on a directory with multiple source files that have the same name / package. How should that be dealt with (if at all?).

@adireddy adireddy changed the title SuppressWarnings flexibility SuppressWarnings flexibility Mar 11, 2016
@adireddy adireddy added the ready label Mar 11, 2016
@adireddy
Copy link
Member

At present, it will ignore all the classes with the same name/package. That's a rare edge case and I think it's ok to ignore all instances.

@Gama11
Copy link
Member

Gama11 commented Mar 11, 2016

Not sure how rare. For any collection of projects (flixel-demos for example), this is going to be common: there are a lot of Main.hx, PlayState.hx etc files. For libraries of course it's going to be rare.

@adireddy
Copy link
Member

Ok makes sense for collection of projects.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 15, 2016

I've been trying this out on dev branch, and have some thoughts.

Firstly, the "check" : ["types"] approach is fine, in case my above example transform of your example wasn't clear - I don't mind it this way at all - I was just doing it for exploring the options.

Secondly, the multiple config file requirement is not ideal. I shouldn't have to litter my repo with multiple files for a single project level configuration. I was hoping the "exclude": { "all":[] } nodes would be a child of the existing checkstyle.json first, and a secondary file could be specified with a matching format so that it's easy to merge them inside the tool code and serve both use cases. For sure, though, two files just for one checkstyle configuration is not fitting.

Thirdly, the packages are currently relative to the -s which breaks the reasoning if you try to just test a single sub package.

With:
checkstyle.json
some/pack/Class.hx

And checkstyle -s some all of the rules for "some.pack" fail now.
In a way this makes sense because of it treating it like the classpath but this is a notable concern for flexibility and testing. I haven't looked closer at the checkstyle.json config, maybe it allows the class path to be specified relative to the rules file or something - but I wanted to point it out so long.

@adireddy
Copy link
Member

Thanks for the feedback @underscorediscovery, will definitely revisit this soon.

@ruby0x1
Copy link
Author

ruby0x1 commented Mar 15, 2016

Another note: It's not possible to use wildcard paths for exclude yet right?

The issue being something like bin/ folder may include build output that includes generated code that isn't in control to resolve checks against. Since I have like 60+ folders with a potential bin/ inside for tests and samples it is impractical to add each path to ignore explicitly, so glob paths or wildcards would be useful.

@adireddy
Copy link
Member

The idea of exclude is to exclude packages, classes in the path(s) specified using -s

Your case the valid when running on samples or collection of projects and I am not yet sure how to solive it :)

@adireddy
Copy link
Member

First things first @underscorediscovery, just added flexibility to specify exclude in the main config or using a separate JSON.

"exclude": {

}

@adireddy
Copy link
Member

Packages are currently relative to the -s like you said.

When running on a package the excludes should be relative. It's a bit weird because of dot(.) notation.

With:
checkstyle.json
some/pack/Class.hx

When running on some, excludes should start with pack instead of some.pack.

@adireddy
Copy link
Member

Tested the above scenario locally and it works.

@adireddy adireddy removed their assignment Mar 16, 2016
AlexHaxe added a commit to AlexHaxe/haxe-checkstyle that referenced this issue May 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants