maven-release icon indicating copy to clipboard operation
maven-release copied to clipboard

Published 20 hours ago • verified publisher icon apache

Simplify PerformReleaseMojo

Open michael-o opened this issue 3 years ago • 7 comments

@kwin @nielsbasjes Have a look whether this makes sense to you. I am not 100% certain about this. It feels awkward that this mojo does not extend from AbstractScmReleaseMojo, but still from AbstractReleaseMojo since other release mojos do so. If this isn't right a comment should be added to the code explaning why.

This affects #133 and #134.

michael-o avatar Jul 30 '22 22:07 michael-o

@slavino Have a look also.

michael-o avatar Jul 30 '22 22:07 michael-o

@kwin @nielsbasjes @slavino I need a review to continue with other open issues.

michael-o avatar Aug 21 '22 18:08 michael-o

I had a look at this code and I agree that there is indeed some duplication that can be simplified this way. As the entire build pipeline passes I think the code change is good. The only "effect" is that you reduce the code duplication (good) and the PerformReleaseMojo will have more config settings available than needed.

nielsbasjes avatar Aug 21 '22 21:08 nielsbasjes

I agree with @nielsbasjes that a lot more parameters are now exposed for goal perform which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between AbstractScmReadReleaseMojo and AbstractScmWriteReleaseMojo (inheriting from the former).

kwin avatar Aug 22 '22 07:08 kwin

I agree with @nielsbasjes that a lot more parameters are now exposed for goal perform which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between AbstractScmReadReleaseMojo and AbstractScmWriteReleaseMojo (inheriting from the former).

As part of #149, this change could possibly be introduced? Unless @michael-o wishes to do that

edwardUL99 avatar Sep 12 '22 07:09 edwardUL99

I'm wondering what the real downside (if any) is of having some properties available that are not used. Apparently (as shown in #149) some of those are actually missing and should be made available.

nielsbasjes avatar Sep 12 '22 07:09 nielsbasjes

For me the 2 phase release process is already complicated enough. Exposing unused parameters for the perform goal in its documentation (https://maven.apache.org/maven-release/maven-release-plugin/perform-mojo.html) will make it even harder to grasp what is going on. Also I expect a lot of wrong reports due to the unused (but documented) parameters.

kwin avatar Sep 12 '22 08:09 kwin

I agree with @nielsbasjes that a lot more parameters are now exposed for goal perform which are not useful. Maybe the AbstractScmReleaseMojo would need a split up between AbstractScmReadReleaseMojo and AbstractScmWriteReleaseMojo (inheriting from the former).

This totally makes sense.

michael-o avatar Oct 29 '22 17:10 michael-o