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

adding skip to apply mojo #367

Merged
merged 3 commits into from
Mar 7, 2019

Conversation

rmannibucau
Copy link
Contributor

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

@nedtwigg
Copy link
Member

nedtwigg commented Mar 7, 2019

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
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
Copy link
Member

nedtwigg commented Mar 7, 2019

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
Copy link
Member

nedtwigg commented Mar 7, 2019

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

@rmannibucau
Copy link
Contributor Author

👍 done

@lutovich
Copy link
Contributor

lutovich commented Mar 7, 2019

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
Copy link
Member

nedtwigg commented Mar 7, 2019

It'll be released by Monday.

@jbduncan
Copy link
Member

jbduncan commented Mar 7, 2019

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
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