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

Exclusive / Negated Globs No Longer Work #999

Closed
thecoden opened this issue Dec 8, 2020 · 6 comments · Fixed by #1061
Closed

Exclusive / Negated Globs No Longer Work #999

thecoden opened this issue Dec 8, 2020 · 6 comments · Fixed by #1061
Assignees
Labels
Milestone

Comments

@thecoden
Copy link

thecoden commented Dec 8, 2020

Expected Behavior

When I run ktlint with **/*.kt & !**/generated/**, all files in any subfolder of a folder named generated should be excluded.

Observed Behavior

After many tests, and reviewing the code, negated patterns are completely ineffective.

Steps to Reproduce

  1. Make a folder called "generated" in your source code, with kt files inside.
  2. Run ktlint with the **/*.kt & !**/generated/** patterns and note that files in the generated folder are still listed.

Your Environment

Gradle, Linux

Solution

This issue was originally caused by the workaround in issue #942 Instead of using the Glob.from(vararg pattern) method, it iterates each pattern, collecting all the matching files, which prevents the exclusion pattern from working.

I have fixed the original issue in the klob dependency so that the workaround is no longer necessary. I will submit a pull request of the correction to ktlint.

@Tapchicoma Tapchicoma self-assigned this Dec 8, 2020
@Tapchicoma Tapchicoma added the bug label Dec 8, 2020
@Tapchicoma Tapchicoma added this to the 0.41.0 milestone Dec 8, 2020
@Tapchicoma
Copy link
Collaborator

Unfortunately I've missed this case in my fix 😒

I am considering replacing klob with standard Java PathMatcher plus FileVisitor.

@thecoden
Copy link
Author

thecoden commented Dec 8, 2020

All good @Tapchicoma . For now, I have published an artifact to jcenter to fix the klob issue and I am prepping a pull request to go back to the simpler code using the new artifact. We also submitted pull requests to the original klob author of course so you could switch back to that dependency should you choose to do so. If you'd like, I will also work on the replacement using the PathMatcher as you mentioned.

@thecoden
Copy link
Author

thecoden commented Dec 9, 2020

In addition to the previous fix, we now have a class that uses PathMatcher and the kotlin File.walkTopDown() method to visit the tree. It uses relative paths and works well, but PathMatcher has some slight differences in behavior compared to Klob.

Mainly, the path a/blah.kt matches the pattern **.kt but does not match the pattern **/*.kt.

We could leave this if you are happy with that behavior or attempt a replacement of */ in patterns, but I can see where it could be convenient to match only files in at least one directory down instead of assuming the root directory matches when you have a / in the pattern.

I have started creating unit tests for this, based on Klob, which is how I discovered the behavior difference. Please let me know how to proceed.

See the following branch / commit:
codetactics@fed22c5

Thank you!

@thecoden
Copy link
Author

thecoden commented Dec 12, 2020

Unfortunately I've missed this case in my fix unamused

I am considering replacing klob with standard Java PathMatcher plus FileVisitor.

I have further updated our Path Matcher / Vistor branch to have better documentation, receive the baseDirectory as a parameter to the getFiles() method, allowing for manual matching against a single path for testing, etc. and, allowed for regex support, which is supported by the standard PathMatcher.

By default, patterns are assumed to be glob, but if you pass a pattern in such as regex:!^.*?/includes?" (note the regex: prefix), it will be interpreted by the PathMatcher as a regular expression. On the ktlint end, we still support the ! to negate the pattern, whether a pattern type prefix is included or not.

Please let us know next steps to move these items forward if you would like to take this direction.

Thank you again for an awesome tool!

@Tapchicoma
Copy link
Collaborator

interesting, I would assume glob: syntax will suit better here. Also according to the docs syntax should be explicitly passed to getPathMatcher(), so no default syntax is set.

Will check your changes and see how it works 👍

@thecoden
Copy link
Author

Hi @Tapchicoma ,

Sorry for the delayed response. Yes, the code defaults to glob: patterns and can only use regex: if the user explicitly puts that prefix. It tooks us a while but we have deployed our own fork and its been working well so far. The biggest thing to be careful of is making sure the users are aware of the path matching change that the standard path matching does differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants