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.

Sign up for GitHub

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

Jump to bottom

[Question] What is the reason to drop support for absolute paths in file patterns? #1131

Closed
petertrr opened this issue Apr 6, 2021 · 12 comments · Fixed by #1199
Closed

[Question] What is the reason to drop support for absolute paths in file patterns? #1131

petertrr opened this issue Apr 6, 2021 · 12 comments · Fixed by #1199
Labels
question
Milestone
0.42.0

Comments

@petertrr
Copy link
Contributor

petertrr commented Apr 6, 2021

I'm seeing the new exception at

ktlint/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/FileUtils.kt

Line 89 in 7e2c7fd

"KtLint does not support absolute path in globs:\n${joinToString(separator = "\n")}"
, as well as a note in release notes that ktlint dropped support for absolute paths in patterns starting from 0.41.0. I understand that this is because of migration to PathMatcher, but I don't quite understand the reason for this particular restriction.
Is it a permanent decision or can this behavior be altered in future releases? Or does it break something in expected workflow?
@romtsn romtsn added the question label Apr 6, 2021
@orchestr7
Copy link
Contributor

My understanding is that some complex platforms that unify different static analyzers can be affected by this change.
In most cases such platforms are using absolute paths to files to unify different static analysis in one system.

Kernald referenced this issue in bazelbuild/rules_kotlin Apr 21, 2021
@shs96c
Introduce ktlint_test and ktlint_fix (#502)
83224e4 
These rules are the first step in providing a lightweight way of
ensuring projects use consistent formatting.
@Kernald Kernald mentioned this issue Apr 26, 2021
Open
@rohandhruva
Copy link

rohandhruva commented May 11, 2021 •
edited
Loading

As mentioned in the issue description, the README and the attached PR#999 is really vague on the details. I might have missed some of the documentation around it, but I also don't see a way to match against a path using the regex: prefix. Was this eventually dropped?

I would also like to know if there are any plans to bring back this functionality. I'm not sure how popular the negated blobs feature was, but it seems like a really expensive bargain if we can no longer specify full path files. This makes it extremely hard to use ktlint in a mono-repo which has other languages and corresponding tools already integrated. The two attached issues from bazelbuild/rules_kotlin seem to be hinting at the same.

Would you please reconsider adding back support for full path matches? Maybe even behind a CLI flag if needed...?

@adsharma
Copy link 

ktlint -F /tmp/foo.kt

fails as well. No globs involved.

@petertrr petertrr reopened this May 20, 2021
macisamuele added a commit to macisamuele/language-formatters-pre-commit-hooks that referenced this issue May 30, 2021
@macisamuele
Update run_autofix_test to not inject absolute paths
d5337dc 
This change is needed because KTLint, starting from version 0.41.0, dropped support for absolute paths (pinterest/ktlint#1131)

While running pre-commit hooks the injected paths are paths relative to GIT_ROOT, so the presence of absolute paths is merely an artifact of testing.
@macisamuele macisamuele mentioned this issue May 30, 2021
Merged
@shashachu
Copy link
Contributor

Hi folks, we'll look into this, as it sounds like it's blocking a number of people from upgrading.

@orchestr7
Copy link
Contributor

orchestr7 commented Jun 25, 2021 •
edited
Loading

Thanks @shashachu! this will be brilliant :)
We can also help with that if needed

@shashachu shashachu added this to the 0.42.0 milestone Jun 29, 2021
@kentr0w
Copy link

kentr0w commented Jul 30, 2021

Is it expected to be fixed in new reales?

@petertrr petertrr mentioned this issue Aug 6, 2021
Merged
2 tasks
@shashachu
Copy link
Contributor

@petertrr 's PR to add back support for absolute paths has been landed to master. Once the SNAPSHOT builds, it'd be good for folks in this thread to test it out to ensure it works.

@petertrr
Copy link
Contributor Author

@shashachu is there an ETA for 0.43.0 (or maybe 0.42.2) release? We have tested the snapshot on our side, it works as expected.

@orchestr7
Copy link
Contributor

orchestr7 commented Aug 24, 2021 •
edited
Loading

0.43.0 will be integrated into diktat, so there will be a double check of the stability of this version

@kirillgroshkov
Copy link

kirillgroshkov commented Sep 20, 2021 •
edited
Loading

Affects us as well. Very tricky this days to pin ktlint to 0.40.0 (version where this was working).

@orchestr7
Copy link
Contributor

orchestr7 commented Sep 20, 2021 •
edited
Loading

@kirillgroshkov Petya has fixed it in #1199 .
Need just to find someone who will release it...

yigit added a commit to androidx/androidx that referenced this issue Sep 27, 2021
@yigit
Use relative paths for ktlint arguments
da89915 
The latest ktlint update removed the support for absolute paths as
arguments.
pinterest/ktlint#1131

Github workflow used to prepend the checkout source to make paths
absolute and now it does not work anymore. This PR
replaces it with `..` since we run ktlint inside `activity` project.

There is probably a better shell magic to get this but I think .. is good
enough for now. (there does not seem to be a consistent solution between
mac and linux for relative paths).

Bug: n/a
Test: locally verified that relative paths with .. works by manually
breaking a file's style in room and invoked ktlint in activity as the
workflow does.
@yigit yigit mentioned this issue Sep 27, 2021
Closed
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Sep 28, 2021
@yigit @copybara-github
[GH] Use relative paths for ktlint arguments
d5c8b4a 
The latest ktlint update removed the support for absolute paths as
arguments.
pinterest/ktlint#1131

Github workflow used to prepend the checkout source to make paths
absolute and now it does not work anymore. This PR
replaces it with `..` since we run ktlint inside `activity` project.

There is probably a better shell magic to get this but I think .. is good
enough for now. (there does not seem to be a consistent solution between
mac and linux for relative paths).

Bug: n/a
Test: locally verified that relative paths with .. works by manually
breaking a file's style in room and invoked ktlint in activity as the
workflow does.

This is an imported pull request from #247.

Resolves #247
Github-Pr-Head-Sha: da89915
GitOrigin-RevId: 80adfcd
Change-Id: Ie6ec09c66a57f89970aff9da581135769b43ca9f
@kirillgroshkov
Copy link

kirillgroshkov commented Oct 20, 2021 •
edited
Loading

Because the fixed version is still not released and our users were reporting this issue over and over, I've wrote down an instruction on how to install ("pin") the working 0.40.0 version (cause it's super-tricky with brew).

Leaving it here in case it's useful for someone else.

Install specific version of ktlint

Currently, the latest version of ktlint is broken (doesn't support absolute urls, while we need that support).

You'll need 0.40.0 version of ktlint.

First, uninstall whatever version you have, with brew uninstall ktlint.

Please go to the github release page for that version: https://github.com/pinterest/ktlint/releases/tag/0.40.0

Download the ktlint binary from there (current download link)

Then, move this ktlint binary from the Downloads folder to /usr/local/bin, e.g like this:

mv ~/Downloads/ktlint /usr/local/bin
chmod +x /usr/local/bin/ktlint

Test that it works: ktlint --version should say 0.40.0

@lars-reimann lars-reimann mentioned this issue Nov 13, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
0.42.0
Development

Successfully merging a pull request may close this issue.

Support absolute paths for globs petertrr/ktlint
8 participants
@shashachu @adsharma @rohandhruva @kirillgroshkov @romtsn @petertrr @kentr0w @orchestr7