-
Notifications
You must be signed in to change notification settings - Fork 50
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
Reload Ktlint's caches when .editorconfig
changes
#265
Conversation
Any progress on this? @jeremymailen @mateuszkwiecinski |
Sorry for the delay. I was on vacation and then sick, so side projects haven't gotten much attention lately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate that we have to work around it in this way, but this seems like a sensible implementation.
Just curious if the support for nested .editorconfig files is comprehensive.
src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt
Show resolved
Hide resolved
return generateSequence(seed = projectEditorConfig) { editorconfig -> | ||
if (editorconfig.isRootEditorConfig) { | ||
null | ||
} else { | ||
editorconfig.parentFile?.parentFile?.resolve(".editorconfig") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation does find .editorconfig
files located in the current directory or in any parent. Also it is nice that you stop visiting parent directories when you encounter the "root=true" property.
What I do miss however, but I might be wrong, is checking for .editorconfig
files inside the project itself. Don't they need to be detected and listed beforehand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I do miss however, but I might be wrong, is checking for .editorconfig files inside the project itself. Don't they need to be detected and listed beforehand?
I think this is a valid comment 👀 Although I'd avoid blindly traversing through project sources looking for potential editorconfig files. I claim most project use single editorconfig defined at Gradle's rootProject
+ in advanced use cases users override its settings by setting another editorconfig
at Gradle project
level (i.e. to have different formatting for generated files committed to the repository).
I'll still suggest going with what I proposed here, at least for now, as I believe it addresses most cases plus most likely it'll be fixed when we switch to the new api you'll introduce for pinterest/ktlint#1446
resetEditorconfigCacheIfNeeded( | ||
wasEditorConfigChanged = parameters.wasEditorConfigChanged, | ||
logger = logger, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of resetting the entire cache of KtLint, we can also add a new API to KtLint that just evicts or directly reloads an editorconfig file based on its pathname. That would be an easy change. As API consumer you will be in full control to respond on changes in .editorconfig
files that you are aware of. Last but not least, it is not needed to reload the entire cache.
I assume we'll update this PR after #275? |
a88d650
to
dcf068b
Compare
dcf068b
to
0c897be
Compare
.editorconfig
changes.editorconfig
changes
Yes exactly. I waited until |
src/main/kotlin/org/jmailen/gradle/kotlinter/support/EditorConfigUtils.kt
Show resolved
Hide resolved
return generateSequence(seed = projectEditorConfig) { editorconfig -> | ||
if (editorconfig.isRootEditorConfig) { | ||
null | ||
} else { | ||
editorconfig.parentFile?.parentFile?.resolve(".editorconfig") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I do miss however, but I might be wrong, is checking for .editorconfig files inside the project itself. Don't they need to be detected and listed beforehand?
I think this is a valid comment 👀 Although I'd avoid blindly traversing through project sources looking for potential editorconfig files. I claim most project use single editorconfig defined at Gradle's rootProject
+ in advanced use cases users override its settings by setting another editorconfig
at Gradle project
level (i.e. to have different formatting for generated files committed to the repository).
I'll still suggest going with what I proposed here, at least for now, as I believe it addresses most cases plus most likely it'll be fixed when we switch to the new api you'll introduce for pinterest/ktlint#1446
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
Fixes #262
It seems like this was the scenario I mentioned in: #246 (comment) 😅
My proposal is to do basically the same thing as ktlint-gradle does 🤷 Maaybe if pinterest/ktlint#1446 gets addressed we'll get appropriate api to know when something changes (or ktlint will be smart enough to track
.editorconfig
changes)