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

exclude paths vs CodeClimate #325

Closed
Gama11 opened this issue Feb 10, 2017 · 26 comments
Closed

exclude paths vs CodeClimate #325

Gama11 opened this issue Feb 10, 2017 · 26 comments
Assignees

Comments

@Gama11
Copy link
Member

Gama11 commented Feb 10, 2017

It looks like exclude in checkstyle.json is not working properly for CodeClimate. There's currently this issue being reported for Flixel:

https://codeclimate.com/github/HaxeFlixel/flixel/issues

This shouldn't happen, because it's specifically excluded in checkstyle.json.

I did a bit of experimenting locally, and it looks like when running the following in the Flixel repo, there are no issues reported:

haxelib run checkstyle -s tests/unit/src

However, with these, there are:

haxelib run checkstyle -s tests
haxelib run checkstyle -s tests/unit/src/flixel/math
haxelib run checkstyle -s tests/unit/src/flixel/math/FlxRandomTest.hx

So it seems like the exclude check only works when -s matches the root package exactly (I guess the exclude path uses the relative path to check packages, rather than parsing the package statement?).

I've made two attempts to fix this by changing the .codeclimate.yml config, but they didn't help:

HaxeFlixel/flixel@7c4309d
HaxeFlixel/flixel@884a5e2

@adireddy
Copy link
Member

Exclude uses relative paths as specified here http://haxecheckstyle.github.io/docs/haxe-checkstyle/setup.html#exclude-packagesclasses

The usage of dot structure may be the problem here as it's easy to misunderstand that with package names.

@Gama11
Copy link
Member Author

Gama11 commented Feb 10, 2017

Ok, but is there a way to configure CodeClimate to run on the "correct relative paths"?.

@adireddy
Copy link
Member

adireddy commented Feb 10, 2017

Also, note that exclude paths work with / structure as well.

To fix the issue you can revert code climate changes and change the exclude path to tests/unit/src/flixel/math/FlxRandomTest

I think it should work. Give it a try and let me know.

@adireddy
Copy link
Member

I will update the docs if it works.

@Gama11
Copy link
Member Author

Gama11 commented Feb 10, 2017

Hm, that kind of conflicts with my local usage of checkstyle where I run it with -s flixel and -s tests/unit/src... The reason I do that is that locally, there will be export directories generated by Lime, which also contain .hx files, which I do not want to check. I guess I'll have to exclude those specifically...

@adireddy
Copy link
Member

Can't think of any other better solution atm. Will keep this open.

@Gama11
Copy link
Member Author

Gama11 commented Feb 10, 2017

Hm, I guess I still don't fully understand the issue, since running checkstyle -s . does not produce any warnings, but checkstyle -s tests does. That doesn't make sense to me, the relative path to "flixel.math.FlxRandomTest" should still be wrong.

@adireddy
Copy link
Member

Are you sure? AFAIK only checkstyle -s tests/unit/src should work. Any other paths will produce warnings.

@Gama11
Copy link
Member Author

Gama11 commented Feb 11, 2017

Looking into it a bit more closely, running checkstyle with -progress stops after 7%. Maybe it dies on some file, and never outputs any error info (it's silent even when using runD.n).

@adireddy
Copy link
Member

Just added path property in exclude config which gives an option to use relative or absolute paths in exclude.

@adireddy
Copy link
Member

Using absolute paths should solve this issue. Let me know otherwise.

"exclude": {
		"path": "absolute",
                ...

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

By absolute paths, you mean paths starting with the drive letter (C:\...), right? That's a nice option to have, but not useful for Flixel. We can't make assumptions about what the haxelib directory is, for instance, or which OS we're working with.

Either way the bigger issue here is that I haven't been able to figure out why checkstyle dies after 7% yet (except that the file that causes issues is FlxBitmapFont.hx). I checked places where checkstyle might swallow errors, but didn't get any more information.

@Gama11 Gama11 reopened this Feb 13, 2017
@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

It should be easy to reproduce if you clone the Flixel repo:

> haxelib run checkstyle -s . -progress

Running Checkstyle v2.1.12 using 20/65 checks on 291 source files...

7% - ./flixel/graphics/frames/FlxBitmapFont.hx

>

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

Oh, I forgot to mention: checkstyle with haxeparser 3.3.0 is probably going to die even earlier for you because of HaxeCheckstyle/haxeparser#38.

@adireddy
Copy link
Member

By absolute paths, I mean the path from where you are running haxe-checkstyle which is ideally the project root folder.

So it should work for all the following cases:

haxelib run checkstyle -s tests
haxelib run checkstyle -s tests/unit/src/flixel/math
haxelib run checkstyle -s tests/unit/src/flixel/math/FlxRandomTest.hx

Your exclude should look like:

"exclude": {
	"path": "relative",
	"AvoidStarImport": [
		"tests/unit/src/flixel/math/FlxRandomTest"
	]
}

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

Oh, I see. That should indeed solve that problem. 👍 Although it might be a bit confusing terminology-wise (I associate something different with "absolute path"). It's really more of a relativeToCwd / relativeToProject?

Would you prefer I open a separate issue for that FlxBitmapFont crash?

@adireddy
Copy link
Member

You are right. Both are relative paths. Should we simply go with pathRelativeToProject: true/false?

BTW, haxelib run checkstyle -s . -progress is running fine on my machine.

Running Checkstyle v2.1.4 using 20/62 checks on 277 source files...

87% - ./tests/unit/src/flixel/math/FlxRandomTest.hx./tests/unit/src/flixel/math/FlxRandomTest.hx:5: characters 0-6 : Warning: Using the ".*" form of import should be avoided
100% - ./tests/unit/src/flixel/util/FlxTimerTest.hx

If you still want to track it, separate issue would be better.

Let's keep this open until we sort out the naming.

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

An "enum" might be more flexible if you wanted to support "real" absolute paths at some point?

@adireddy
Copy link
Member

enum it is then.

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

Aka "json enum" / "enum for poor people, which is really just a string value". :D

@adireddy
Copy link
Member

😄

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

BTW, haxelib run checkstyle -s . -progress is running fine on my machine.

Hm, that's quite strange... I wonder what makes the difference there. I'm assuming we're using the same Haxe and Neko versions (Haxe 3.4.0 and Neko 2.1.0 respectively). Different OS probably? Windows 10 here.

@adireddy
Copy link
Member

macOS Sierra here.

@AlexHaxe
Copy link
Member

same here on Linux, so potentially a Windows issue?

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

Wow, you may be right.. I can't reproduce it in Bash on Windows either. I have no idea where to even start with debugging this one... I guess isolating it would help.

The only thing that seems special about that file is this variable with a very long initializer: https://github.com/HaxeFlixel/flixel/blob/dev/flixel/graphics/frames/FlxBitmapFont.hx#L31

And sure enough, it doesn't die anymore if I remove that. I'll open a separate issue.

@Gama11
Copy link
Member Author

Gama11 commented Feb 13, 2017

#331

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

3 participants