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

Possibly bug with Params editorconfig handling #605

Closed
ZacSweers opened this issue Oct 7, 2019 · 5 comments
Closed

Possibly bug with Params editorconfig handling #605

ZacSweers opened this issue Oct 7, 2019 · 5 comments

Comments

@ZacSweers
Copy link

I've been working on adding editorconfig support to the Spotless gradle plugin, but I believe I'm hitting an issue due to this line:

https://github.com/pinterest/ktlint/blob/master/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt#L178

If the input file name doesn't match (such as if it's null), it will ignore the editorconfig that was passed in and return an empty map. My sense is that this was an oversight and it should just return the parsed mapping just without the file_name added to it. Before I make a PR I wanted to confirm if this was the case, or understand better why if it's intentional.

This presents an issue for spotless because it doesn't currently pass in the file name to the format call, just the contents. It might be possible to change this if need be (and maybe should be the case for other rules that depend on file_name), but that's the current reason this can't be simply worked around.

@Tapchicoma
Copy link
Collaborator

Before moving .editorconfig processing to core package this piece of code looked following:

if (editorConfigPath != null) {
    val userData = (
        EditorConfig.of(File(editorConfigPath).canonicalPath)
            ?.onlyIf({ debug }) { printEditorConfigChain(it) }
            ?: emptyMap<String, String>()
        ) + cliUserData
    return fun (fileName: String) = userData + ("file_path" to fileName)
}

and fileName in ktlint cli is always no-null (stdin has special value). Though in ktlint-core fileName param is nullable. Changing it to be non-nullable will be a breaking change. Not sure if mapping without fileName could be used as a solution.

@paul-dingemans
Copy link
Collaborator

Most likely this issue is obsolete due to changes in ktlint 0.46.x.

@paul-dingemans paul-dingemans added this to the 0.47.0 milestone Jun 26, 2022
@paul-dingemans
Copy link
Collaborator

@ZacSweers How relevant is your question at this moment? I have been reading the spotless issue. If I understand correctly, the goal is that when Spotless invokes ktlint that .editorconfig is supported by ktlint.

As you remarked in comment, ktlint needs to know the path of the file so that it can check which .editorconfig files are located on the path to that file. The files are combined hierarchally where files deeper in the hierarchy are more important.

In case ExperimentalParams.editorConfigPath is specified, all '.editorconfig' files on the path to the file are being ignored. Only the setting from the ExperimentalParams.editorConfigPath are read and used.

I am open for suggestions to improve the API of ktlint but at this moment I can not figure out what Spotless's needs are given the current state of both Spotless and KtLint.

One possibily could be that the singular ExperimentalParams.editorConfigPath is changed to a list of paths to .editorConfig where files are ranked from most important to least important. This would allow spotless to collect and track the status of the files.

Another possibility is that an API is added which, given a directory path creates an instance of the EditorConfigOverride based on the '.editorConfig' files which then can be supplierd to calls of 'lint and format for all files in the directory.

@ZacSweers
Copy link
Author

Sorry I no longer use ktlint, so I don't think I have any helpful input

@paul-dingemans
Copy link
Collaborator

Sorry I no longer use ktlint, so I don't think I have any helpful input

Ok, tnx for replying.

I have updated the spotless issue to inform other participants about status in ktlint. Closing this issue for now.

@paul-dingemans paul-dingemans closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2022
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