You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/12/20 21:10:08 UTC
[GitHub] [maven-ear-plugin] elharo opened a new pull request #32: remove deprecated methods by not stubbing value objects
elharo opened a new pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32
@mabrarov
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r546500365
##########
File path: src/test/java/org/apache/maven/plugins/ear/AbstractEarTestBase.java
##########
@@ -33,88 +35,42 @@
public static final String DEFAULT_GROUPID = "eartest";
Review comment:
Could be:
```java
protected static final String DEFAULT_GROUP_ID = "eartest";
```
like `org.apache.maven.plugins.ear.util.ArtifactRepositoryTest#MAIN_ARTIFACT_ID`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#issuecomment-750788357
I'll need next 24-48 hrs to complete with review of this PR. I haven't forgotten about it :) Just busy with my job duties.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r548865908
##########
File path: src/test/java/org/apache/maven/plugins/ear/util/ArtifactRepositoryTest.java
##########
@@ -34,14 +36,15 @@
extends AbstractEarTestBase
{
- public static final String MAIN_ARTIFACT_ID = "none";
+ private static final String MAIN_ARTIFACT_ID = "none";
+
+ private ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
@Test
public void testEmptyRepository()
{
- ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
- ArtifactRepository repo =
- new ArtifactRepository( createArtifacts( null ), MAIN_ARTIFACT_ID, artifactTypeMappingService );
+ Set<Artifact> artifacts = new HashSet<>();
Review comment:
Won't `Collections#emptySet` be more readable? Like:
```java
ArtifactRepository repo =
new ArtifactRepository( Collections.<Artifact>emptySet(), MAIN_ARTIFACT_ID, artifactTypeMappingService );
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r548865908
##########
File path: src/test/java/org/apache/maven/plugins/ear/util/ArtifactRepositoryTest.java
##########
@@ -34,14 +36,15 @@
extends AbstractEarTestBase
{
- public static final String MAIN_ARTIFACT_ID = "none";
+ private static final String MAIN_ARTIFACT_ID = "none";
+
+ private ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
@Test
public void testEmptyRepository()
{
- ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
- ArtifactRepository repo =
- new ArtifactRepository( createArtifacts( null ), MAIN_ARTIFACT_ID, artifactTypeMappingService );
+ Set<Artifact> artifacts = new HashSet<>();
Review comment:
Do we really need `artifacts` local variable and isn't `Collections#emptySet` be more readable? Like:
```java
ArtifactRepository repo =
new ArtifactRepository( Collections.<Artifact>emptySet(), MAIN_ARTIFACT_ID, artifactTypeMappingService );
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo merged pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r546667976
##########
File path: src/test/java/org/apache/maven/plugins/ear/AbstractEarTestBase.java
##########
@@ -33,88 +35,42 @@
public static final String DEFAULT_GROUPID = "eartest";
Review comment:
done
##########
File path: src/test/java/org/apache/maven/plugins/ear/AbstractEarTestBase.java
##########
@@ -33,88 +35,42 @@
public static final String DEFAULT_GROUPID = "eartest";
- public static final String DEFAULT_TYPE = "jar";
+ private static final String DEFAULT_TYPE = "jar";
protected void setUri( EarModule module, String uri )
{
( (AbstractEarModule) module ).setUri( uri );
}
- protected Set<Artifact> createArtifacts( String[] artifactsId )
+ protected Set<Artifact> createArtifacts( String[] artifactIds )
{
- return createArtifacts( artifactsId, null );
+ return createArtifacts( artifactIds, null );
}
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types )
- {
- return createArtifacts( artifactsId, types, null );
- }
-
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types, String[] groupsId )
- {
- return createArtifacts( artifactsId, types, groupsId, null );
- }
-
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types, String[] groupsId,
- String[] classifiers )
+ protected Set<Artifact> createArtifacts( String[] artifactIds, String[] classifiers )
{
Set<Artifact> result = new TreeSet<Artifact>();
- if ( artifactsId == null || artifactsId.length == 0 )
- {
- return result;
- }
- for ( int i = 0; i < artifactsId.length; i++ )
+ ArtifactHandlerTestStub artifactHandler = new ArtifactHandlerTestStub( "jar" );
+ for ( int i = 0; i < artifactIds.length; i++ )
{
- String artifactId = artifactsId[i];
- String type = getData( types, i, DEFAULT_TYPE );
- String groupId = getData( groupsId, i, DEFAULT_GROUPID );
- String classifier = getData( classifiers, i, null );
- result.add( new ArtifactTestStub( groupId, artifactId, type, classifier ) );
+ String artifactId = artifactIds[i];
+ String classifier = classifiers == null ? null : classifiers[i];
+ Artifact artifactTestStub = new DefaultArtifact(
+ DEFAULT_GROUPID, artifactId, "1.0", "compile", DEFAULT_TYPE, classifier, artifactHandler );
+
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r548976754
##########
File path: src/test/java/org/apache/maven/plugins/ear/util/ArtifactRepositoryTest.java
##########
@@ -34,14 +36,15 @@
extends AbstractEarTestBase
{
- public static final String MAIN_ARTIFACT_ID = "none";
+ private static final String MAIN_ARTIFACT_ID = "none";
+
+ private ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
@Test
public void testEmptyRepository()
{
- ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
- ArtifactRepository repo =
- new ArtifactRepository( createArtifacts( null ), MAIN_ARTIFACT_ID, artifactTypeMappingService );
+ Set<Artifact> artifacts = new HashSet<>();
Review comment:
IMHO nested functions are far less readable. There's no name to explain what the argument is. The line is longer and more likely to run off the edge of the screen or have to be wrapped. And it's definitely more trouble to work with in a debugger. Short, simple lines that do exactly one thing are my preference.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r548865908
##########
File path: src/test/java/org/apache/maven/plugins/ear/util/ArtifactRepositoryTest.java
##########
@@ -34,14 +36,15 @@
extends AbstractEarTestBase
{
- public static final String MAIN_ARTIFACT_ID = "none";
+ private static final String MAIN_ARTIFACT_ID = "none";
+
+ private ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
@Test
public void testEmptyRepository()
{
- ArtifactTypeMappingService artifactTypeMappingService = new ArtifactTypeMappingService();
- ArtifactRepository repo =
- new ArtifactRepository( createArtifacts( null ), MAIN_ARTIFACT_ID, artifactTypeMappingService );
+ Set<Artifact> artifacts = new HashSet<>();
Review comment:
Do we really need `artifacts` local variable and isn't `Collections#emptySet` more readable? Like:
```java
ArtifactRepository repo =
new ArtifactRepository( Collections.<Artifact>emptySet(), MAIN_ARTIFACT_ID, artifactTypeMappingService );
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #32: remove deprecated methods by not stubbing value objects
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #32:
URL: https://github.com/apache/maven-ear-plugin/pull/32#discussion_r546499679
##########
File path: src/test/java/org/apache/maven/plugins/ear/AbstractEarTestBase.java
##########
@@ -33,88 +35,42 @@
public static final String DEFAULT_GROUPID = "eartest";
- public static final String DEFAULT_TYPE = "jar";
+ private static final String DEFAULT_TYPE = "jar";
protected void setUri( EarModule module, String uri )
{
( (AbstractEarModule) module ).setUri( uri );
}
- protected Set<Artifact> createArtifacts( String[] artifactsId )
+ protected Set<Artifact> createArtifacts( String[] artifactIds )
{
- return createArtifacts( artifactsId, null );
+ return createArtifacts( artifactIds, null );
}
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types )
- {
- return createArtifacts( artifactsId, types, null );
- }
-
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types, String[] groupsId )
- {
- return createArtifacts( artifactsId, types, groupsId, null );
- }
-
- protected Set<Artifact> createArtifacts( String[] artifactsId, String[] types, String[] groupsId,
- String[] classifiers )
+ protected Set<Artifact> createArtifacts( String[] artifactIds, String[] classifiers )
{
Set<Artifact> result = new TreeSet<Artifact>();
- if ( artifactsId == null || artifactsId.length == 0 )
- {
- return result;
- }
- for ( int i = 0; i < artifactsId.length; i++ )
+ ArtifactHandlerTestStub artifactHandler = new ArtifactHandlerTestStub( "jar" );
+ for ( int i = 0; i < artifactIds.length; i++ )
{
- String artifactId = artifactsId[i];
- String type = getData( types, i, DEFAULT_TYPE );
- String groupId = getData( groupsId, i, DEFAULT_GROUPID );
- String classifier = getData( classifiers, i, null );
- result.add( new ArtifactTestStub( groupId, artifactId, type, classifier ) );
+ String artifactId = artifactIds[i];
+ String classifier = classifiers == null ? null : classifiers[i];
+ Artifact artifactTestStub = new DefaultArtifact(
+ DEFAULT_GROUPID, artifactId, "1.0", "compile", DEFAULT_TYPE, classifier, artifactHandler );
+
Review comment:
It looks like there are trailing spaces in this line.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org