-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fixes for #349 #350
Fixes for #349 #350
Conversation
Changed the excludes to use the string as a regular expression except in the case where a "path" option is provided. Where a path is provided, it should work as before. Removed the appended class name because the simpler provided path will still match paths checked against it.
Compilation on Travis is failing: https://travis-ci.org/HaxeCheckstyle/haxe-checkstyle/jobs/266828167#L305-L308 |
My bad! Some last minute refactoring I didn't check. |
|
Ok now I'm not too sure why the build is failing: haxe: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.17' not found (required by haxe) |
Don't worry about libc.so.6, it seems to be a problem with the development version of Haxe and / or Travis's Haxe support. |
That can be fixed by adding I'll do that in a separate PR. |
Was just about to do that, but you were faster... :) |
Codecov Report
@@ Coverage Diff @@
## dev #350 +/- ##
==========================================
- Coverage 89.78% 89.78% -0.01%
==========================================
Files 116 116
Lines 7331 7330 -1
Branches 293 293
==========================================
- Hits 6582 6581 -1
Misses 489 489
Partials 260 260
Continue to review full report at Codecov.
|
@AlexHaxe for now I've got it so it uses straight regex if you don't specify "path" option in inside checkstyle.json. This may break existing configurations. Would you prefer we rather add a new option e.g. "regex": "true" or something like that, or is my approach acceptable? |
If we introduce a breaking change, then users have to change their config files. We would have to communicate it with the next release. I am not opposed to a breaking change - I personally don't use exclusions in my checkstyle.json (mainly because I use one config for many projects), so I am not personally affected. And we should make checkstyle not emit checkstyle warnings during build (either by fixing issues or by using limited exclusions). |
I don't think many users use excludes which is probably why this is only now becoming an issue. We need to apply different rules for tests vs source code. For example, tests may need to use long functions to mock out objects, etc. We don't want a separate checkstyle.json for tests because all the other rules are still shared. The problem with using the excludes as-is is that you can't filter on a class name at all. It seems you can only specify a different package. I suppose we could get away with just refactoring how the paths work but regex is a nice to have and is actually pretty simple vs. the other "path" options. |
Checkstyle supports a Also the current state of exclusion handling is a work in progress implementation of #120 that became a quasi "permanent" solution. I think before #120 you could only suppress checks using inline annotations e.g. For the sake of this pull request I would say we update checkstyle.json so our build passes without checkstyle issues. |
Changed the excludes to use the string as a regular expression except in
the case where a "path" option is provided. Where a path is provided, it
should work as before. Removed the appended class name because the
simpler provided path will still match paths checked against it.