-
Notifications
You must be signed in to change notification settings - Fork 458
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
adding skip to apply mojo #367
Conversation
There is a misunderstanding between @rmannibucau and I which we have not been able to resolve.
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:
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. |
@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). |
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 ;-) |
Add a bullet to the maven changelog and I'll merge. |
👍 done |
Imho this feature is not unreasonable but I wouldn't want to use it :) |
It'll be released by Monday. |
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. :) |
Released in 1.19.0 |
Follow up of #366 hoping it can be integrated even if spotless wouldn't recommend that.