-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Refine AspectJ plugin build phase config #3284
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
Conversation
ebf31dd to
37bf519
Compare
Document the pros and cons in the POM with this comment: Attention: aspectj-maven-plugin MUST be declared AFTER maven-compiler-plugin, if they are both supposed to run in the same default phases 'compile' and 'test-compile', but execution order is to be guaranteed. Then, you have the convenience of being able to run e.g. 'mvn [clean] compile' instead of 'mvn [clean] process-classes'. For a slightly less convenient, but less refactoring-error-prone solution, see the commented-out phases below.
37bf519 to
6e28a9f
Compare
@mp911de, like I said over there in the other PR, after your ANTLR optimisation and removal of Replacer plugin, I dropped the first commit because it is now obsolete, leaving only the AspectJ Maven phase change to default. I still think, this would help more than it would hurt. It is documented well enough to avoid anyone messing up in the future. And if they would mess up, they would notice immediately, because the build would break.
Alright, we're on the same page now. Care to remove the commented-out XML bits so I can merge the PR without polishing it? Happy to keep the explanatory comment though.
Actually, I left them there on purpose, and the comment even points to them. I like things to be explicit rather than expecting other developers to possess sacred knowledge. But if you insist, of course I can remove them, as soon as I am near my workstation again. 🙂
OK, done, even though I still think that keeping the phases and the hint as a help for the future would be better. You can either merge the whole PR with both commits or cherry-pick just the first one, if you change your mind.
52ee55f to
61e6d36
Compare
Uh oh!
There was an error while loading. Please reload this page.
This is a follow-up on #3282, where a reviewer's misunderstanding (running test builds with wrong Maven phases) led to parts of the PR getting falsely erased. This PR
(削除) not only reinstates those erased parts, but also (削除ここまで)enables developers who do not know the build configuration well enough to know that they should runmvn process-classesrather thanmvn compileto now use the latter. This new convenience feature is not a fix or change of the previous PR, but changes a build phase which has been there long before the PR.@mp911de, @odrotbohm