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 editor config file option to maven and gradle #1442

Merged
merged 17 commits into from
Jan 9, 2023

Conversation

eirnym
Copy link
Contributor

@eirnym eirnym commented Jan 2, 2023

Fixes #142

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this long-requested feature! I have just a few nits

@eirnym
Copy link
Contributor Author

eirnym commented Jan 3, 2023

I've set the default for editorConfigPath for Gradle (for ${rootProject.projectDir}/.editorconfig", as rootProject is the very distinct place.

I'm not quite sure, what to set to maven.

Some SO answers suggest to use session.executionRootDirectory (which works only under some curcumstances), some answers suggest to set $basedir in CI and so on. Thus I'll just ommit default for maven.

What do you think?

@eirnym eirnym requested a review from nedtwigg January 3, 2023 22:17
@nedtwigg
Copy link
Member

nedtwigg commented Jan 3, 2023

What do you think?

I would either do

File candidateDir = AbstractSpotlessMojo.baseDir;
while (candidateDir != null && candidateDir doesn't have .editorConfig && candidateDir doesn't have .git) {
  candidateDir = candidateDir.getParentFile()
}

or I would leave the default empty.

@eirnym
Copy link
Contributor Author

eirnym commented Jan 3, 2023

or I would leave the default empty.

I'll leave it as it is then (no default).

@eirnym
Copy link
Contributor Author

eirnym commented Jan 5, 2023

@nedtwigg now it should work properly. I checked only Gradle part. I've tested with KtLint 0.47.1 and KtLint 0.48.1

I don't know how to substitute where maven take source code for a plugin, so can you check it somehow?

PS: one review note in GitHub UI I can't mark as resoved.

@eirnym eirnym mentioned this pull request Jan 7, 2023
@nedtwigg
Copy link
Member

nedtwigg commented Jan 8, 2023

You are unblocked, and if any other conflicts come through you've got first right-of-way :). No rush.

@eirnym
Copy link
Contributor Author

eirnym commented Jan 8, 2023

@davidburstrom, @nedtwigg I merged the main branch with #1452.

I already used new interface for the processing and tested it before, so I left it as well. Thanks to @Sineaggi there's no deprecated method usages for the KtLint 0.48 interface (beside the disabled_rules).

I believe, that (basic) 0.49 support will be the a copy of this with disabled rules removed if nothing else will be changed in the interface.

* Use java.nio.file.Files.exists to be consistent
* Use empty empty EditorConfigDefaults settings for KtLint 0.47 and 0.48
  when possible
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all, this looks great! Regarding the discussions on star imports and File vs Path, the Spotless style guide is, as @davidburstrom so succinctly put it:

if it's not enforced automatically, you shouldn't have to worry about it.

@eirnym, please confirm you are ready to merge, and I will merge and release.

@eirnym
Copy link
Contributor Author

eirnym commented Jan 9, 2023

@nedtwigg Yes, I feel feature is ready to be merged

@nedtwigg nedtwigg merged commit 4ee53aa into diffplug:main Jan 9, 2023
@eirnym eirnym deleted the feat/ktlint-editor-config branch January 9, 2023 18:42
nedtwigg added a commit that referenced this pull request Jan 9, 2023
nedtwigg added a commit that referenced this pull request Jan 9, 2023
nedtwigg added a commit that referenced this pull request Jan 9, 2023
…et picked up which will change formatting result
nedtwigg added a commit that referenced this pull request Jan 9, 2023
…ing license header, so step order matters.
nedtwigg added a commit that referenced this pull request Jan 9, 2023
@nedtwigg
Copy link
Member

nedtwigg commented Jan 9, 2023

@eirnym & @davidburstrom just FYI, this PR had a lot of breakage as-merged. I fixed it in the commits referenced above, you can see them all together in #1474.

The culprit is that CircleCI has become very flaky and stopped checking commits in this PR for some reason. We'll fix that eventually in

I should have noticed that, and if I had I would have asked for this PR to be closed and then reopened fresh to trigger CircleCI.

The second biggest contributing factor is that KtLint isn't maintaining a stable public API. The whole point of Spotless is that formatting doesn't matter - formatting API's really don't matter, I'm quite frustrated that ktlint is so much work to keep up with.

And then the third factor is that even a small change (.editorconfig) requires changing eight files with more-or-less the same change. Code like that is super hard to review well, because the changes get monotonous. As you can see in the commits above, about half of the KtLint adapters were inoperable due to NPE's, but that was impossible to see. Luckily there were tests, but unluckily the tests weren't running.

I personally don't like KtLint, I think KtFmt does a better job of "formatting doesn't matter", so I'm happy to merge whatever the community wants to use because I'm not gonna use KtLint regardless. But I just wanted to give a heads up that if somebody submitted a PR to drop support for all but the latest KtLint, I would probably merge it. The current approach is quite fragile. IMO projects should always get easier to contribute to over time, and this PR makes me worry that our KtLint integration is becoming sclerotic.

@nedtwigg
Copy link
Member

nedtwigg commented Jan 9, 2023

Sorry - I should have also added that the primary thing I should express here is gratitude to you both for contributing useful things to the project! Lots of people like and use ktlint, and the PRs you have submitted help all of those people!

The collective system of you, me, KtLint, and CircleCI came together to almost ship a release which would have been very broken for KtLint users, so my comment above is intended only as a technical post-mortem.

The full experience that end users actually have is that Spotless and KtLint work very well together because of your efforts, and thanks to this PR they work together even better than they used to! So thanks :)

@eirnym
Copy link
Contributor Author

eirnym commented Jan 9, 2023

@nedtwigg thank you for fixing things up. I saw the errors and learned how it's different from Gradle API with holders, which are resolved only at the end task or worker. I learn from my mistakes and will test more thoroughly.

I currently prefer KtLint and I'd like to drop some old versions first (#1475)

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.

Add support for ktlint to respect .editorconfig
4 participants