Conversation
|
@slavino Have a look also. |
|
@kwin @nielsbasjes @slavino I need a review to continue with other open issues. |
|
I had a look at this code and I agree that there is indeed some duplication that can be simplified this way. |
| if ( username != null ) | ||
| { | ||
| releaseDescriptor.setScmUsername( username ); | ||
| } | ||
|
|
||
| if ( password != null ) | ||
| { | ||
| releaseDescriptor.setScmPassword( password ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Sure this should be removed?
Now the releaseDescriptor no longer has these values.
There is no scenario when during the perform release the username/password are needed?
If there is no such scenario: Why do you keep the scm source url? I the ScmSourceUrl is needed then I would suspect the username and password are also needed in some cases.
There was a problem hiding this comment.
This is IMHO already part of AbstractScmReleaseMojo.createReleaseDescriptor()
|
I agree with @nielsbasjes that a lot more parameters are now exposed for goal |
As part of #149, this change could possibly be introduced? Unless @michael-o wishes to do that |
|
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. |
|
For me the 2 phase release process is already complicated enough. Exposing unused parameters for the |
kwin
left a comment
kwin
left a comment
There was a problem hiding this comment.
Please separate reading from SCM (perform) and writing to SCM (prepare) into two separate abstract classes.
This totally makes sense. |
|
FTR: The separation between scm read and read/write goals was now applied in 7f60b4e. |
4 participants
@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 fromAbstractReleaseMojosince 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.