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

Add ability to specify editorconfig path #207

Merged
merged 2 commits into from
Jun 14, 2018

Conversation

charbgr
Copy link
Contributor

@charbgr charbgr commented May 5, 2018

  • Adds the ability to user to specify the .editorconfig path.

The problem:

I setup ktlint in my project and then I installed the pre-commit hook. "Real" code is under cliffhanger/ folder and the .editorconfig. is placed under that folder.
The issue is that pre-commit hook is being installed in the parent git directory (where not "Real" code exists) and the .editorconfig is placed in a different folder, resulting to wrong lint warnings.

With this fix I can modify my pre-commit hook and set editoconfig path by

$ git diff --name-only --cached --relative | grep '\.kt[s"]\?$' | xargs ktlint --relative . --editorconfig-path="cliffhanger/"

Let me know what you think and looking forward to your constructive feedback!

@charbgr charbgr force-pushed the charbgr/custom-editorconfig-path branch from 888893f to e593dcf Compare May 5, 2018 23:07
@shyiko
Copy link
Collaborator

shyiko commented May 7, 2018

Hi @charbgr.

How about we change .editorconfig lookup logic so that instead of checking only the workdir it would check the directory of every matched file?

editorconfig.org:

Where are these files stored?
When opening a file, EditorConfig plugins look for a file named .editorconfig in the directory of the opened file and in every parent directory. A search for .editorconfig files will stop if the root filepath is reached or an EditorConfig file with root=true is found.

I realise it would require 1 more stat (2)(dir/.editorconfig) syscall for every new (sub)directory (unless klob is modified to expose this) and that the results will need to be cached to avoid excessive I/O but I believe making ktlint behave closer to the way editors (e.g. VSCode) do is the way to go here.

What do you think? Would you be willing to give it a try?

@charbgr
Copy link
Contributor Author

charbgr commented May 7, 2018

Sure, I will give a try!

What if I find two or more .editorconfigs in different directories? Should ktlint behave different for each one?

@shyiko
Copy link
Collaborator

shyiko commented May 7, 2018

Yes.

Given

/project/.editorconfig # let's say indent_size = 4
/project/a/.editorconfig # indent_size = 2
/project/a/A.kt
/project/b/.editorconfig # indent_size = 8
/project/b/B.kt
/project/c/C.kt

A.kt should observe indent_size = 2, B.kt indent_size = 8 and C.kt - indent_size = 4.

https://github.com/shyiko/ktlint/blob/master/ktlint/src/main/kotlin/com/github/shyiko/ktlint/internal/EditorConfig.kt#L19

@charbgr
Copy link
Contributor Author

charbgr commented May 23, 2018

Hi again,

sorry for being late on this. I was busy these weeks.

I pushed my first implementation for custom .editorconfig per filepath. My solution doesn't require klob to expose anything.

What I did is that I am iterating files found from klob and for each of them I am going backwards till I find the working directory(workDir). Once an .editorconfig is found, I put it in a Map with an EditorConfig.

Let me know what you think!

@charbgr charbgr force-pushed the charbgr/custom-editorconfig-path branch 2 times, most recently from 9e81478 to 726b0a7 Compare May 23, 2018 17:59
@charbgr charbgr force-pushed the charbgr/custom-editorconfig-path branch from 726b0a7 to 88beb05 Compare May 23, 2018 18:01
@charbgr
Copy link
Contributor Author

charbgr commented Jun 12, 2018

Hey @shyiko,

did you have the time to check the last update?

@shyiko
Copy link
Collaborator

shyiko commented Jun 14, 2018

Sorry, I've been busy the last couple of weeks. Merging in! 👍

@shyiko shyiko merged commit 1360289 into pinterest:master Jun 14, 2018
@bethcutler
Copy link
Contributor

Is there any chance you might reconsider the flag to specify the configuration file?

In our use case, we are attempting to create a consistent style across a broad spectrum of projects. Prior to this change, it was possible to enforce compliance by having one .editorconfig in the directory from which ktlint was run. After this change, it is possible for any project to circumvent the configuration just by adding their own .editorconfig file in their directory.

@shyiko
Copy link
Collaborator

shyiko commented Jul 12, 2018

@bethcutler sure. PR is welcome.

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

Successfully merging this pull request may close these issues.

None yet

3 participants