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

Skip execution in incremental (m2e) builds by default #2059

Merged
merged 7 commits into from
Apr 7, 2024

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Mar 2, 2024

Clear stale messages during "check" mojo
Allow to parameterize message severity
Add Spotless prefix to messages

This closes #1814
This closes #2037

Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

Allow edits from maintainers

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

  • a summary of the change
  • either
    • a link to the issue you are resolving (for small changes)
    • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

@kwin
Copy link
Contributor Author

kwin commented Mar 18, 2024

@nedtwigg Anything I can do to speed up the review process?

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Great PR, sorry it sat for a while.

Do you think it's okay to prefix these flags with m2e? I want to make sure they don't add confusion for non-eclipse users.

plugin-maven/README.md Outdated Show resolved Hide resolved
plugin-maven/README.md Outdated Show resolved Hide resolved
@kwin
Copy link
Contributor Author

kwin commented Apr 3, 2024

Do you think it's okay to prefix these flags with m2e? I want to make sure they don't add confusion for non-eclipse users.

I only implement an m2e agnostic API, namely https://github.com/codehaus-plexus/plexus-build-api. However the only known user of that API is currently m2e. Still I would rather lean towards keeping the m2e agnostic names, because at least theoretically it could be used by any IDE/incremental build tool....

@nedtwigg
Copy link
Member

nedtwigg commented Apr 3, 2024

If another IDE comes along and implements the standard we can add an alias for that IDE too, but in the meantime if the only users today or in the immediate future is m2e, I don't want to get support requests from users of other IDEs.

@kwin kwin requested a review from nedtwigg April 3, 2024 16:22
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Thanks for making the change. Looking at the docs now, it seems like we can make this simpler to use and document by dropping down to only one parameter.


```
<configuration>
<m2eEnableForIncrementalBuild>true</m2eEnableForIncrementalBuild><!-- this is false by default -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<m2eEnableForIncrementalBuild>true</m2eEnableForIncrementalBuild><!-- this is false by default -->
<m2eIncrementalBuild>WARNING</m2eIncrementalBuild><!-- this is NONE by default, can also be ERROR -->

Copy link
Member

Choose a reason for hiding this comment

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

or maybe m2eIncrementalBuildMessages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m2eIncrementalBuild is too fuzzy for me, m2eIncrementalBuildMessages sounds better.

Copy link
Member

Choose a reason for hiding this comment

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

That works for me :)

Copy link
Contributor Author

@kwin kwin Apr 4, 2024

Choose a reason for hiding this comment

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

After having a second thought, I think having only one parameter is over-simplifying things, as we talk about two different goals:

  1. apply
  2. check

The message severity is only ever useful for the latter, while m2eEnableForIncrementalBuild applies to both.

Copy link
Member

Choose a reason for hiding this comment

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

If m2eEnableForIncrementalBuild is false, then m2eIncrementalBuildMessages has no effect, correct? And if m2eEnableForIncrementalBuild is true, would it ever make sense for m2eIncrementalBuildMessageSeverity to be NONE or SILENT?

I might be wrong, but this seems like having a separate switch for the fuel pump on a car.

Copy link
Contributor Author

@kwin kwin Apr 4, 2024

Choose a reason for hiding this comment

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

If m2eEnableForIncrementalBuild is false, then m2eIncrementalBuildMessages has no effect, correct?

That is correct.

And if m2eEnableForIncrementalBuild is true, would it ever make sense for m2eIncrementalBuildMessageSeverity to be NONE or SILENT?

No, but the point is that noone expects that m2eIncrementalBuildMessageSeverity or m2eIncrementalBuildMessages would toggle off the incremental build at all.

but this seems like having a separate switch for the fuel pump on a car.

For me having one multivalue switch feels more like adjusting your AC will implicitly also start your car ;-)

Copy link
Member

Choose a reason for hiding this comment

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

That's not such a bad idea if the car can't run without A/C. But whatever, I usually defer to the people who are gonna use the thing.

Only thing missing now is a changelog entry.

Copy link
Contributor Author

@kwin kwin Apr 4, 2024

Choose a reason for hiding this comment

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

Done

P.S. I would consider m2eIncrementalBuildMessageSeverity advanced setting (rarely adjusted) while m2eEnableForIncrementalBuild is a basic setting (easy to understand for everyone, more often adjusted). Hope this is enough justification for keeping both separate, but this is definitely my last argument to this discussion :-)

plugin-maven/README.md Outdated Show resolved Hide resolved
@nedtwigg nedtwigg merged commit f7346d5 into diffplug:main Apr 7, 2024
14 checks passed
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.

Add option to disable m2e problem reporting Unfortunate automated execution in Eclipse via m2e
2 participants