michael-o commented on code in PR #135:
URL: https://github.com/apache/maven-release/pull/135#discussion_r884286863
##########
maven-release-manager/pom.xml:
##########
@@ -238,9 +238,7 @@
<executions>
<execution>
<goals>
- <goal>xpp3-reader</goal>
Review Comment:
No model necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -560,15 +559,10 @@ private ReleaseDescriptorBuilder
loadReleaseDescriptorBuilder( ReleaseDescriptor
{
try
{
- updateListener( listener, "verify-release-configuration",
PHASE_START );
- ReleaseDescriptorBuilder result = configStore.get().read( builder
);
- updateListener( listener, "verify-release-configuration",
PHASE_END );
- return result;
+ return configStore.get().read( builder );
}
catch ( ReleaseDescriptorStoreException e )
{
- updateListener( listener, e.getMessage(), ERROR );
Review Comment:
Why is this information not necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -616,8 +608,6 @@ public void clean( ReleaseCleanRequest cleanRequest )
throws ReleaseFailureExcep
( (ResourceGenerator) phase ).clean(
cleanRequest.getReactorProjects() );
}
}
-
- updateListener( cleanRequest.getReleaseManagerListener(), "cleanup",
PHASE_END );
Review Comment:
Why is this information not necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/CheckoutProjectFromScm.java:
##########
@@ -270,13 +270,19 @@ public ReleaseResult simulate( ReleaseDescriptor
releaseDescriptor, ReleaseEnvir
{
ReleaseResult result = new ReleaseResult();
+ MavenProject rootProject = ReleaseUtil.getRootProject( reactorProjects
);
+ File checkoutDirectory =
+ FileUtils.resolveFile( rootProject.getBasedir(),
releaseDescriptor.getCheckoutDirectory() );
+
if ( releaseDescriptor.isLocalCheckout() )
{
- logInfo( result, "This would be a LOCAL check out to perform the
release ..." );
+ logInfo( result,
+ "This would be a LOCAL check out to perform the release
from " + checkoutDirectory + "..." );
Review Comment:
I prefer a strong on `local` rather than shouting it out load in upper case.
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -726,7 +732,10 @@ private void logInfo( ReleaseResult result, String message
)
private void captureException( ReleaseResult result,
ReleaseManagerListener listener, Exception e )
{
- updateListener( listener, e.getMessage(), ERROR );
+ if ( listener != null )
+ {
+ listener.error( e.getMessage() );
Review Comment:
Pity that exception object is lost
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java:
##########
@@ -199,9 +201,15 @@ private void transform( ReleaseDescriptor
releaseDescriptor, ReleaseEnvironment
{
result.setStartTime( ( startTime >= 0 ) ? startTime :
System.currentTimeMillis() );
+ URI root = ReleaseUtil.getRootProject( reactorProjects
).getBasedir().toURI();
+
for ( MavenProject project : reactorProjects )
{
- logInfo( result, "Transforming '" + project.getName() + "'..." );
+ URI pom = project.getFile().toURI();
+ logInfo( result,
+ "Transforming " + root.relativize( pom ).getPath() + ' '
Review Comment:
I believe that `URI` is not necessary here, but `Path` can make it relative
too, no?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRunGoalsPhase.java:
##########
@@ -71,11 +72,12 @@ protected ReleaseResult execute( ReleaseDescriptor
releaseDescriptor, ReleaseEnv
String goals = getGoals( releaseDescriptor );
if ( !StringUtils.isEmpty( goals ) )
{
- logInfo( result, "Executing goals '" + goals + "'..." );
- if ( logArguments && ( additionalArguments != null ) )
+ logInfo( result, "Executing goals '" + buffer().strong( goals
) + "'..." );
Review Comment:
Why not `#mojo()`?
##########
maven-release-plugin/src/site/apt/examples/perform-release.apt.vm:
##########
@@ -29,7 +29,7 @@ Perform a Release
Performing a release runs the following release phases
{{{../../maven-release-manager/#perform}by default}}:
- * Checkout from an SCM URL with optional tag
+ * Checkout from an SCM URL with optional tag to <<<workingDirectory>>>
(<<<target/checkout>>> by default)
Review Comment:
I'd prefer to use the official property instead of default `target`.
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/RunPerformGoalsPhase.java:
##########
@@ -99,7 +101,7 @@ private ReleaseResult runLogic( ReleaseDescriptor
releaseDescriptor, ReleaseEnvi
{
ReleaseResult result = new ReleaseResult();
- logInfo( result, "Simulating perform goals '" + getGoals(
releaseDescriptor )
+ logInfo( result, "Simulating perform goals '" + buffer().strong(
getGoals( releaseDescriptor ) )
Review Comment:
Why not `#mojo()`?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -592,8 +586,6 @@ protected void clean( AbstractReleaseRequest releaseRequest
) throws ReleaseFai
@Override
public void clean( ReleaseCleanRequest cleanRequest ) throws
ReleaseFailureException
{
- updateListener( cleanRequest.getReleaseManagerListener(), "cleanup",
PHASE_START );
Review Comment:
Why is this information not necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManagerListener.java:
##########
@@ -86,16 +86,9 @@ public void goalStart( String goal, List<String> phases )
public void phaseStart( String name )
{
- if ( goal == null || ( ( currentPhase + 1 ) >= phases.size() ) )
Review Comment:
Why is this information not necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -560,15 +559,10 @@ private ReleaseDescriptorBuilder
loadReleaseDescriptorBuilder( ReleaseDescriptor
{
try
{
- updateListener( listener, "verify-release-configuration",
PHASE_START );
- ReleaseDescriptorBuilder result = configStore.get().read( builder
);
- updateListener( listener, "verify-release-configuration",
PHASE_END );
- return result;
Review Comment:
Why is this information not necessary anymore?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRunGoalsPhase.java:
##########
@@ -71,11 +72,12 @@ protected ReleaseResult execute( ReleaseDescriptor
releaseDescriptor, ReleaseEnv
String goals = getGoals( releaseDescriptor );
if ( !StringUtils.isEmpty( goals ) )
{
- logInfo( result, "Executing goals '" + goals + "'..." );
- if ( logArguments && ( additionalArguments != null ) )
+ logInfo( result, "Executing goals '" + buffer().strong( goals
) + "'..." );
+ if ( logArguments )
{
// logging arguments may log secrets: should be activated
only on dryRun
- logInfo( result, " with additional arguments: " +
additionalArguments );
+ logInfo( result, " with additional arguments: "
+ + ( additionalArguments == null ? "" :
additionalArguments ) );
Review Comment:
The purpose is to show whether there are addArgs or not? Consistency?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
