-
-
Notifications
You must be signed in to change notification settings - Fork 76
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 spotless #538
add spotless #538
Conversation
<configuration> | ||
<java> | ||
<endWithNewline /> | ||
<importOrder /> |
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.
@timja this import order might mess with how we have configured in checkstyle, but we could simply adapt to the order so that static was on top.
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.
heh low level of caring what the actual order is as long as it's defined and enforced in code :).
happy to change when we update the parent pom
I was so confused by this; to clarify: This is indentation of the pom, correct? |
https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#sortpom |
@@ -54,6 +54,18 @@ If you had a `jar:test-jar` execution, delete it and add to `properties`: | |||
<no-test-jar>false</no-test-jar> | |||
``` | |||
|
|||
To fix spotless validation errors: |
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.
may want to mention this: https://github.com/jenkinsci/github-branch-source-plugin/blob/master/.git-blame-ignore-revs
(may be more appropriate in release notes)
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.
Sure, we can add it in the readme and the release notes.
This looks really great. Thanks for starting this effort. I will try this on a few of the plugins I maintain and see how it works. I want to explore the various settings we expose to users. In order to avoid disruption, I think any new functionality should be off by default, unless the vast majority of existing plugin POMs already comply. And as far as opting in to new functionality, I think there are two types of users:
Whatever interface we come up with should support both types of users, I think. For example, core has accepted import management and POM ordering from Spotless, but I doubt the core maintainers would be receptive to formatting every Java source file with Google Java Format. In contrast, Liam Newman did reformat one of his plugins with Google Java Format, and he was OK with the "diff wall". So we need to keep both types of users in mind in order for such a change to be adopted without complaints. |
git blame --ignore-revs-file .git-blame-ignore-revs |
I know, and I also use |
@@ -116,6 +116,8 @@ | |||
<scmTag>HEAD</scmTag> | |||
<!-- Where to put temporary files during test runs. --> | |||
<surefireTempDir>${project.build.directory}/tmp</surefireTempDir> | |||
<spotless.version>2.22.1</spotless.version> | |||
<sort.nrOfIndentSpace>2</sort.nrOfIndentSpace> |
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.
<sort.nrOfIndentSpace>2</sort.nrOfIndentSpace> | |
<sortPom.nrOfIndentSpace>2</sort.nrOfIndentSpace> |
Maybe? (+ downstream changes)
Or even
<sort.nrOfIndentSpace>2</sort.nrOfIndentSpace> | |
<spotless.sortPom.nrOfIndentSpace>2</sort.nrOfIndentSpace> |
to make it really obvious what this is related to.
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.
I am fine with
<sort.nrOfIndentSpace>2</sort.nrOfIndentSpace> | |
<spotless.sortPom.nrOfIndentSpace>2</spotless.sortPom.nrOfIndentSpace> |
Regarding git blame, I suggest creating a PR afterwards to avoid commit sha changing on us in this PR if we use squash or rebase. But definitely a good suggestion :) |
@basil my only concern if we open up the property list to fully configure each little aspect of spotless we will have 20-30 new properties. My suggestion is to only have |
Apparently dependency order matters when it comes to commons-logging: see #132 @jtnord do you have any suggestion on your TODO, how should this be fixed so that order did not matter? <dependency>
<!-- to avoid https://www.slf4j.org/codes.html#release warning -->
<!-- TODO this would be better as a managed version of [0] -->
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
<scope>provided</scope>
</dependency> |
Yeah, I definitely don't think that the interface between the plugin parent POM and plugin POMs should expose every internal Spotless tunable that exists. That would be impossible to support in the long term as Spotless tunables evolve upstream. But I am not sure that an "all or nothing" approach of all Spotless checks or none of them is ideal either. For one thing, the existing "all" implementation in this PR doesn't satisfy me, as it doesn't run Google Java Format on every Java source file. :-) But to give another perspective, I think the percentage of people that are excited enough by this sort of thing to enable the "all" mode are relatively few, and I think the vast majority of people would stay in the "none" mode to avoid having to do any work. I think we might be able to curate a "middle ground" of less disruptive changes that might appeal to more people with a minimum amount of effort. If that "middle ground" is undisruptive enough, we could even consider enabling it by default. |
I take it you mean test files? Yes, we should 😄
I guess getting consensus for that would change over time, we should be prepared for changes to the settings than. |
I'm saying that if I wanted to apply Spotless to my plugins, by "opting in" to some mode (whatever we decide to call it or name the property), I'd want it to run SortPom with maximum awesomeness (e.g., enforcing 2-space indentation, dependency sorting, all the good stuff) and Google Java Format with maximum awesomeness (e.g., full reformatting of every Java file in |
The way I have been thinking about this in my mind is that we could define some "profiles" for formatting: e.g. "none" (no Spotless at all), "low" (some changes, like eliminating unused imports in Java code and conservative changes to |
Profiles might be a good suggestion 🤔 |
I think implementation wise it would be pretty easy, too: we could define a Jenkins-specific property to determine which profile to use, and then define several different Maven profiles with the different spotless settings, with the activation for each Maven profile being based on the value of the Jenkins-specific property. |
Oh right I keep forgetting that Maven profile activation works with system properties and not with Maven properties. But the gist of my idea is the same. You could instead enable or disable the Maven profiles for the different "Jenkins formatting profiles" in |
Order matters period. it is not just about commons-logging but this is how maven and dependencies work. if you have if you declare your dependencies as Then you have the Also you have the classpath order which is dependant (possibly) on the dependency order. TL;DR enforcing a sort order of dependencies is incorrect.
could possibly be |
IIRC this approach does not work for multi-module projects or aggregators, where you require different config per module. |
<endWithNewline /> | ||
<importOrder /> | ||
<indent> | ||
<spaces>true</spaces> |
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.
tabs ;-p
<spaces>true</spaces> | |
<spaces>false</spaces> |
</java> | ||
<pom> | ||
<sortPom> | ||
<encoding>${project.build.sourceEncoding}</encoding> |
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.
POMs are XML and will be UTF-8 by default not always the sourceEncoding?
if the spotless pom parser can not handle XML correctly and detect the encoding - please raise a bug against it.
<pom> | ||
<sortPom> | ||
<encoding>${project.build.sourceEncoding}</encoding> | ||
<lineSeparator>\n</lineSeparator> |
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 would probably a lot of windows setups where the line ending will get converted.
(not mine as I use as-is
- but possibly many others).
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.
</pom> | ||
</configuration> | ||
<executions> | ||
<execution> |
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.
should have an id so it can be changed / overridden correctly in child projects?
</configuration> | ||
<executions> | ||
<execution> | ||
<!-- Runs in verify phase by default --> |
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.
TBH - this is one of the worst things. you run the tests everything passes locallay then 20+ minutes later CI says no just because of some trailing space...
Should be run much earlier in the lifecycle IMO (e.g. validate) and be disabled with the quick-build
profile (which it is)
given this is a breaking change (it will cause builds to fail, and I guess many of them are not formatted as per the rules) I think it should be opt-in (then there is the question of how this impacts plugins that currently have it enabled. (with DB most people probably ignore any "upgrade notes" unless a build fails, and in the case we disable spotless unless you then set a flag, it could cause spotless to be silently disabled). I am not against enabling spotless, but if different plugin authors all require different settings, is having the configuration in here the best solution? |
Closing in favor of #733. |
Implement spotless with sane defaults. I don't think we need to go down the rabbit hole of configuration.
I added an example for
<sort.nrOfIndentSpace>2</sort.nrOfIndentSpace>
I would like to remove it simply keep sane defaults and than if someone dislike it they can either override the configuration or use
spotless.check.skip
to skip.At the moment this opt-out via
<spotless.check.skip>true</spotless.check.skip>
If there is any desire we can also make it opt-in by setting it to default
<spotless.check.skip>false/spotless.check.skip>
Not sure who should be pinged to properly review this.
@basil