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

Expose information about files/resources that will be accessed when invoking ktlint #1446

Closed
mateuszkwiecinski opened this issue Apr 5, 2022 · 5 comments · Fixed by #1659

Comments

@mateuszkwiecinski
Copy link
Contributor

Originally requested in: #1434 (comment)

Gradle Tasks should explicitly list their inputs. When a plugins like ktlint-gradle or kotlinter-gradle want to use ktlint they try to guess which resources will be used by ktlint, i.e. which .editorconfig files will be used : https://github.com/JLLeitschuh/ktlint-gradle/blob/ddd465e28d77b879384886e1eef5666ebe518b4d/plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/PluginUtil.kt#L56
Missing input declaration leads to bugs like jeremymailen/kotlinter-gradle#233 and as they try to guess and mimic ktlint behavior the new code becomes susceptible to bugs like JLLeitschuh/ktlint-gradle#575.
I'm not sure what I'm asking specifically, but it would be nice to have a set of public utilities to identify ktlint dependencies (filesktlint will access when invoked?).

Expected Behavior

As a ktlint Gradle wrapper maintainer, I know which files will be accessed before actually invoking Ktlint#lint

Current Behavior

Currently, there is no way to know which files/resources ktlint will read when invoked, which results in plugins trying to guess which files should be observed by Gradle to ensure build correctness.

Additional information

  • Current version of ktlint: all

If I'm not mistaken, ktlint uses external dependency, EditorConfigLoader which automagically reads all related properties for a given file location, but from Gradle Plugin perspective specific properties don't matter, so it can't be really used.

@paul-dingemans
Copy link
Collaborator

I think, it would be not that difficult to provide a new API. Do I understand correctly that the request API would look something like:

input: path to a source file
output: list of .editorconfig files on the path to file which will be read when the source file is scanned.

I have two concerns though:

  • In order to determine whether a parent directory has to be scanned, it is required to read the .editorconfig. Only in case it does not include property root=true the parent directory may be scanned. Would this have any impact on you as API consumer?
  • The additional call to KtLint might have performance impact as more file operations are conducted.

@mateuszkwiecinski
Copy link
Contributor Author

Hey @paul-dingemans! Sorry for a late reply 😬

In order to determine whether a parent directory has to be scanned, it is required to read the .editorconfig. Only in case it does not include property root=true the parent directory may be scanned. Would this have any impact on you as API consumer?

As you probably saw my take on working around the issue, that's exactly what my proposal does, and I believe reading files is the only reasonable option, that's how editorconfig files work 🤷

The additional call to KtLint might have performance impact as more file operations are conducted.

I'm not sure if I fully understood this point. Your concern is about the new API methods you consider exposing? Or will it affect "regular" Ktlint#lint or #format calls? If the former - I don't believe the performance impact would be noticeable. It would be used by Gradle wrappers, called during Gradle configuration phase which eventually will be cached, which eventually will be cached and input files will be observed by Gradle watchig mechanism.

Regarding the API proposal:

  1. Are the .editorconfig the only inputs of Ktlint? You proposed to expose .editorconfig files ktlint depends on, are there any other types of files it would make sense to observe?
  2. Knowing both Gradle plugins configure project per Gradle sourceSet (which means they share ktlint configuration for all files under src/main/kotlin), will it be possible to query for ktlint "inputs" for a given directory, not just a file?
    I.e. Ktlint.listAllInputs(fileOrDirectory: File): List<File> (or more future-proof wrapper class like class KtlintInputs(val editorconfigFiles: List<File>, val someOtherInputType: List<File>))
  3. In context of your comment here: will you also consider adding a way to reload .editorconfig configuration when Gradle reports changes to their content? That seems to be worth including to make sure Gradle plugins can correctly interact with Ktlint instance.

My availability is limited recently, but I'll be happy to give an early feedback and test all changes you're about to propose :)

@paul-dingemans
Copy link
Collaborator

@mateuszkwiecinski

I'm not sure if I fully understood this point. Your concern is about the new API methods you consider exposing? Or will it affect "regular" Ktlint#lint or #format calls? If the former - I don't believe the performance impact would be noticeable. It would be used by Gradle wrappers, called during Gradle configuration phase which eventually will be cached, which eventually will be cached and input files will be observed by Gradle watchig mechanism.

The new API method will access the file system. The regularKtlint#lint and #format calls will not be changed and will still access the filesystem as before. So the additional performance penalty is solely the call to the new API method.

Regarding the API proposal:

  1. Are the .editorconfig the only inputs of Ktlint? You proposed to expose .editorconfig files ktlint depends on, are there any other types of files it would make sense to observe?

Only files that are read by ktlint are the .editorconfig files and of course the source files themselves.

  1. Knowing both Gradle plugins configure project per Gradle sourceSet (which means they share ktlint configuration for all files under src/main/kotlin), will it be possible to query for ktlint "inputs" for a given directory, not just a file?
    I.e. Ktlint.listAllInputs(fileOrDirectory: File): List<File> (or more future-proof wrapper class like class KtlintInputs(val editorconfigFiles: List<File>, val someOtherInputType: List<File>))

The .editorconfig file is always specific to a directory. So specifying a path to a file or a directory will have same result. Biggest difference of course would be that for a file we only need to traverse towards the root dir of the filesystem to find parent .editorconfig files. In case a directory is specified, it would be necessary to traverse all subdirectories underneath as well as any of those directories can contain .editorconfig file. But given that this call only needs to be executed once for the rootdirectory of the project, it should not have too much impact.

  1. In context of your comment here: will you also consider adding a way to reload .editorconfig configuration when Gradle reports changes to their content? That seems to be worth including to make sure Gradle plugins can correctly interact with Ktlint instance.

This actually has already been released in KtLint 0.47.0.

My availability is limited recently, but I'll be happy to give an early feedback and test all changes you're about to propose :)

Sure, no problem.

@paul-dingemans paul-dingemans added this to the 0.48.0 milestone Sep 4, 2022
@mateuszkwiecinski
Copy link
Contributor Author

All clear then! Thank you 🙏

This actually has already been released in KtLint 0.47.0.

Nice, I clearly focused too much on other api changes and missed that part of the release notes. Thanks for pointing that out. I can confirm I tested that locally and it works in an expected way 🚀

paul-dingemans added a commit to paul-dingemans/ktlint that referenced this issue Sep 24, 2022
@paul-dingemans
Copy link
Collaborator

@mateuszkwiecinski Can you please have a look at the PR (at least at the changelog)?

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

Successfully merging a pull request may close this issue.

2 participants