Skip to content

adding skip to apply mojo - #367

Merged
nedtwigg merged 3 commits into
diffplug:masterfrom
rmannibucau:adding-skip-to-apply-mojo
Mar 7, 2019
Merged

adding skip to apply mojo#367
nedtwigg merged 3 commits into
diffplug:masterfrom
rmannibucau:adding-skip-to-apply-mojo

Conversation

@rmannibucau

Copy link
Copy Markdown
Contributor

Follow up of #366 hoping it can be integrated even if spotless wouldn't recommend that.

@nedtwigg

nedtwigg commented Mar 7, 2019

Copy link
Copy Markdown
Member

There is a misunderstanding between @rmannibucau and I which we have not been able to resolve.

  • If I'm correct that @rmannibucau is misunderstanding the interaction between check/apply and CI, then this is a needless change but it's small
  • If I'm wrong, then @rmannibucau is able to solve their problem

Small downside if I'm right, large upside if I'm wrong. This makes me inclined to assume I'm wrong and merge. I'd like to hear one other voice besides you and me to break the tie. This voice doesn't have to convince me, they just need to agree with you and I'll yield.

In summary:

  1. your build runs apply and check by default
  2. I think this is a mistake - a default build is slowed down by running both apply and check, yet CI cannot catch formatting mistakes - all downside and no upside
  3. If you don't want builds to fail, then you should run neither apply nor check by default, rather than running them both
  4. Many past users of Spotless have been confused by this - they think that by running apply by default, then their code will stay formatted, but this is surprisingly not the case (see 2). This feature has been requested several times in the gradle plugin, and it has always been because of a misunderstanding of this surprising interaction between running apply by default and CI. The feature is missing on purpose, because users are been better off understanding that enabling apply by default does not do what they hoped it would do.

Can you have a colleague read this summary and agree with you in this PR? Perhaps explaining to someone in person will help break our logjam, and at the very least if they post here that they agree then I will merge and release in the next version. Alternatively, I'd be fine if @lutovich overrides me and presses merge.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

@nedtwigg 1 is not what I do and likely the misunderstanding part, I just apply (and never check since I run apply all the time) ;). This was the compromise we picked since we always (ok ok 95% ;)) build locally before pushing to have sources well formatted and not have to run each time we write a line a mvn command (check leads to that and this is a real pain).

@nedtwigg

nedtwigg commented Mar 7, 2019

Copy link
Copy Markdown
Member

Aha! Fair enough. You've convinced me that you understand what you're signing up for. As long as one understands the downside of hanging oneself, I won't stop you from having rope ;-)

@nedtwigg

nedtwigg commented Mar 7, 2019

Copy link
Copy Markdown
Member

Add a bullet to the maven changelog and I'll merge.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

👍 done

@lutovich

lutovich commented Mar 7, 2019

Copy link
Copy Markdown
Contributor

Imho this feature is not unreasonable but I wouldn't want to use it :)
It is possible to skip spotless:apply with a Maven profile but "native" support via a property makes it more convenient. Other plugins like spotbugs and checkstyle have skip flags though both are more like spotless:check.

@nedtwigg
nedtwigg merged commit 6b864a3 into diffplug:master Mar 7, 2019
@nedtwigg

nedtwigg commented Mar 7, 2019

Copy link
Copy Markdown
Member

It'll be released by Monday.

@jbduncan

jbduncan commented Mar 7, 2019

Copy link
Copy Markdown
Member

Am I right to understand that the reason why running both "apply" and "check" together at CI time is usually considered bad is because after "apply" runs, the formatted results don't get checked into version control?

If my understanding is correct, then I'm reasonably happy for this PR to be merged, as I can tell that @rmannibucau and others have considered the tradeoffs of introducing this flag. :)

@rmannibucau
rmannibucau deleted the adding-skip-to-apply-mojo branch March 11, 2019 07:50
@nedtwigg

Copy link
Copy Markdown
Member

Released in 1.19.0

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.

4 participants