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

Consolidate skip behavior #969

Merged
merged 1 commit into from
Oct 14, 2021
Merged

Consolidate skip behavior #969

merged 1 commit into from
Oct 14, 2021

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Oct 13, 2021

Fix for failure of plugin when both ratchetFrom and skip=true are specified if no Git repository is present, as is sometimes the case for CI or Docker builds.

Fix for #968

@nedtwigg nedtwigg merged commit 13a0e42 into diffplug:main Oct 14, 2021
@jherico jherico deleted the brad_fix-skip branch October 14, 2021 00:54
@nedtwigg
Copy link
Member

Thanks for the fix! Published in 2.17.2.

@Parameter(defaultValue = "${mojoExecution.goal}", required = true, readonly = true)
private String goal;

@Parameter(property = "spotless.check.skip", defaultValue = "false")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might actually need to be spotless.skip?

@lutovich
Copy link
Contributor

Hey @jherico, @nedtwigg! Sorry, I might be a bit late with my comment :)

I think this might be a backward-incompatible change. Before this change, the plugin had two user-facing properties spotless.check.skip and spotless.apply.skip to skip respective goals. With this change, the spotless.apply.skip property becomes a no-op, and spotless.check.skip influences the behavior of the apply goal.

What do you think about keeping both spotless.apply.skip and spotless.check.skip for backward compatibility? The logic of actually skipping files can still live in AbstractSpotlessMojo.

nedtwigg added a commit that referenced this pull request Oct 19, 2021
@nedtwigg
Copy link
Member

I just reverted this PR in main and gave the following reason in the changelog:

because fixing the skip bug caused inconsistent behavior between check.skip and apply.skip.

I am extremely unknowledgeable about maven. I apologize to you both, @lutovich and @jherico, I should not have merged this PR without consulting @lutovich since he is the author of the maven plugin. I try to get people's stuff merged and released quickly and I knew that @lutovich was working on #935 and I wanted to take care of this for him.

Not a big deal, but clearly I made it a tad more complicated for you both :) If you still want this change @jherico, please open a new PR and when @lutovich has time he can review it. 100% my fault, but I'm happy with the outcome because at least @jherico has a published version that works in the meantime, and I'm sure the fix in this PR will make it into main at some point :)

@lutovich
Copy link
Contributor

@nedtwigg no problem at all! Thanks for Handling this PR. I do not intend to be the central authority for the maven plugin. I simply noticed this PR when rebasing #935 :)

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.

None yet

3 participants