Re: Please avoid non-backward compatible API changes - Was: Re: svn commit: r617944 - in /maven/artifact/trunk/src: main/java/org/apache/maven/artifact/ main/java/org/apache/maven/artifact/deployer/ main/java/org/apache/maven/artifact/factory/ main/java/

Sun, 03 Feb 2008 09:50:18 -0800

I rolled them back for the time being and put them on a branch as I'm headed out today. I'll put an aspect in, and then add a test for the artifact handler use in maven-artifact and make an IT like one of the plugins. I've got a few branches here in GIT, that didn't go in correctly but I still would have never have seen the use in plugins in any automated way which is bad. 

On 3-Feb-08, at 3:45 AM, Milos Kleint wrote:


Hello Jason,

Please revert this change or make it backward comaptible. Removing
getArtifactHandler() from Artifact interface might be conceptually
right, but it breaks lots of thing.

1. a few tests are broken in the maven components.
2. More important is that every plugin have ever made that call will
break with a nasty NoSuchMethodException when run in 2.1.
A quick search in mojo.codehaus.org reveals that
appassembler-maven-plugin,
aspectj-maven-plugin,
cobertura-maven-plugin,
commons-attributes-maven-plugin,
native-maven-plugin,
taglist-maven-plugin
and webstart-maven-plugin are all affected.
(Done by grepping the current trunk, not counting stuff from sandbox)
3.You've also broken my code in the netbeans integration that uses the
Artifact's getArtifactHandler method as well. I can rewrite my own
code against the latest embedder, however point 2. still applies.
People won't be able to run many projects with the embedded version of
maven and will have to use some 2.0.x command line version. And of
course everyone will blame me :)


A more general note.
The light-heartedness of making non-backward compatible changes in
maven components  and the core code in general gives me nightmares.
Plugins can reach any component from the maven core via the
${components.*} property, the MavenProject instance via ${project.*},
the MavenSession and Settings also via the property evaluation.
Whoever added this, effectively commited to making all those
components backward compatible public API. You might get away with
adding methods to the interfaces (it's plugins responsibility to track
when the method was added and setup <mavenPrerequisite>
approproately).
But the removal of any method or changing the signature is unacceptable. 

Thanks.

Milos

On Feb 3, 2008 5:19 AM,  <[EMAIL PROTECTED]> wrote:

Author: jvanzyl
Date: Sat Feb  2 20:19:44 2008
New Revision: 617944

URL: http://svn.apache.org/viewvc?rev=617944&view=rev
Log:
o The Artifact should serve as datum, or representation of resource in a Maven repository. It currently has way too much information, and also has references to components. In a first pass I have decoupled the ArtifactHandler from Artifact so that it is no longer required. This means that the primary requirement of the ArtifactFactory is gone and you can now use a simple constructor to create an 
 Artifact.

Modified:
maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/Artifact.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/Artifact.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java Sat Feb 2 20:19:44 2008 
@@ -119,8 +119,6 @@

    void setDependencyFilter( ArtifactFilter artifactFilter );

-    ArtifactHandler getArtifactHandler();
-
    List getDependencyTrail();

    void setDependencyTrail( List dependencyTrail );
@@ -144,9 +142,6 @@
    boolean isResolved();

    void setResolvedVersion( String version );
-
-    /** @todo remove, a quick hack for the lifecycle executor */
-    void setArtifactHandler( ArtifactHandler handler );

    boolean isRelease();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/ArtifactUtils.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ArtifactUtils.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java Sat Feb 2 20:19:44 2008 
@@ -150,10 +150,10 @@
        {
range = VersionRange.createFromVersion( artifact.getVersion() ); 
        }
-
+
DefaultArtifact clone = new DefaultArtifact( artifact.getGroupId(), artifact.getArtifactId(), range.cloneOf(), - artifact.getScope(), artifact.getType(), artifact.getClassifier(), 
-            artifact.getArtifactHandler(), artifact.isOptional() );
+ artifact.getScope(), artifact.getType(), artifact.getClassifier(), artifact.isOptional() ); 
+
        clone.setRelease( artifact.isRelease() );
        clone.setResolvedVersion( artifact.getVersion() );
        clone.setResolved( artifact.isResolved() );
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/DefaultArtifact.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/DefaultArtifact.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java Sat Feb 2 20:19:44 2008 
@@ -37,7 +37,7 @@
import java.util.regex.Matcher;

/**
- * @author <a href="mailto:[EMAIL PROTECTED]">Jason van Zyl </a>
+ * @author Jason van Zyl
 * @version $Id$
 * @todo this should possibly be replaced by type handler
 */
@@ -48,13 +48,6 @@

    private String artifactId;

-    /**
- * The resolved version for the artifact after conflict resolution, that has not been transformed. 
-     *
-     * @todo should be final
-     */
-    private String baseVersion;
-
    private final String type;

    private final String classifier;
@@ -63,28 +56,40 @@

    private File file;
+ // Why is this here? What repository is determined at runtime and is therefore a 
+    // runtime charactistic. This needs to go. jvz.
    private ArtifactRepository repository;

    private String downloadUrl;

+    // Why is this here? jvz.
    private ArtifactFilter dependencyFilter;

-    private ArtifactHandler artifactHandler;
-
+    // Why is this here? jvz?
    private List dependencyTrail;

+    /**
+ * The resolved version for the artifact after conflict resolution, that has not been transformed. 
+     *
+     * @todo should be final
+     */
+    private String baseVersion;
+
    private String version;

    private VersionRange versionRange;

    private boolean resolved;

+    // This is specific to maven. jvz.
    private boolean release;
+ // If the version is stored here (above), why on earth do we store the available versions here? jvz. 
    private List availableVersions;

    private Map metadataMap;

+    // This is Maven specific. jvz/
    private boolean optional;

    public DefaultArtifact( String groupId,
@@ -92,10 +97,9 @@
                            VersionRange versionRange,
                            String scope,
                            String type,
-                            String classifier,
-                            ArtifactHandler artifactHandler )
+                            String classifier )
    {
- this( groupId, artifactId, versionRange, scope, type, classifier, artifactHandler, false ); + this( groupId, artifactId, versionRange, scope, type, classifier, false ); 
    }

    public DefaultArtifact( String groupId,
@@ -104,7 +108,6 @@
                            String scope,
                            String type,
                            String classifier,
-                            ArtifactHandler artifactHandler,
                            boolean optional )
    {
        this.groupId = groupId;
@@ -115,17 +118,10 @@

        selectVersionFromNewRangeIfAvailable();

-        this.artifactHandler = artifactHandler;
-
        this.scope = scope;

        this.type = type;

-        if ( classifier == null )
-        {
-            classifier = artifactHandler.getClassifier();
-        }
-
        this.classifier = classifier;

        this.optional = optional;
@@ -474,11 +470,6 @@
        dependencyFilter = artifactFilter;
    }

-    public ArtifactHandler getArtifactHandler()
-    {
-        return artifactHandler;
-    }
-
    public List getDependencyTrail()
    {
        return dependencyTrail;
@@ -561,11 +552,6 @@
    {
        this.version = version;
        // retain baseVersion
-    }
-
- public void setArtifactHandler( ArtifactHandler artifactHandler ) 
-    {
-        this.artifactHandler = artifactHandler;
    }

    public void setRelease( boolean release )
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/deployer/DefaultArtifactDeployer.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/deployer/DefaultArtifactDeployer.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java Sat Feb 2 20:19:44 2008 
@@ -20,6 +20,7 @@
 */

import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.manager.WagonManager;
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org .apache.maven.artifact.metadata.ArtifactMetadataRetrievalException; 
@@ -63,6 +64,9 @@
    /** @plexus.requirement role-hint="default" */
    private ArtifactRepositoryLayout defaultLayout;

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
/** @deprecated we want to use the artifact method only, and ensure artifact.file is set correctly. */ 
    public void deploy( String basedir,
                        String finalName,
@@ -71,7 +75,8 @@
                        ArtifactRepository localRepository )
        throws ArtifactDeploymentException
    {
- String extension = artifact.getArtifactHandler().getExtension(); + String extension = artifactHandlerManager .getArtifactHandler( artifact.getType() ).getExtension(); 
+
File source = new File( basedir, finalName + "." + extension ); deploy( source, artifact, deploymentRepository, localRepository ); 
    }
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/factory/DefaultArtifactFactory.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/factory/DefaultArtifactFactory.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java Sat Feb 2 20:19:44 2008 
@@ -211,9 +211,6 @@
            desiredScope = Artifact.SCOPE_SYSTEM;
        }
- ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( type ); 
-
- return new DefaultArtifact( groupId, artifactId, versionRange, desiredScope, type, classifier, handler, 
-            optional );
+ return new DefaultArtifact( groupId, artifactId, versionRange, desiredScope, type, classifier, optional ); 
    }
}
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/installer/DefaultArtifactInstaller.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/installer/DefaultArtifactInstaller.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java Sat Feb 2 20:19:44 2008 
@@ -20,6 +20,7 @@
 */

import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;
import org .apache .maven .artifact .repository.metadata.RepositoryMetadataInstallationException; 
@@ -46,6 +47,9 @@
    /** @plexus.requirement */
    private RepositoryMetadataManager repositoryMetadataManager;

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
/** @deprecated we want to use the artifact method only, and ensure artifact.file is set correctly. */ 
    public void install( String basedir,
                         String finalName,
@@ -53,7 +57,8 @@
                         ArtifactRepository localRepository )
        throws ArtifactInstallationException
    {
- String extension = artifact.getArtifactHandler().getExtension(); + String extension = artifactHandlerManager .getArtifactHandler( artifact.getType() ).getExtension(); 
+
File source = new File( basedir, finalName + "." + extension ); 

        install( source, artifact, localRepository );
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/DefaultRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/DefaultRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -21,6 +21,7 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;

@@ -37,9 +38,12 @@

    private static final char ARTIFACT_SEPARATOR = '-';

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/FlatRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/FlatRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -2,9 +2,9 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;
-import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout; 

/**
* The code in this class is taken from DefaultRepositorylayout, located at: 
@@ -20,9 +20,12 @@

    private static final char GROUP_SEPARATOR = '.';

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/LegacyRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/LegacyRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -21,6 +21,7 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;

@@ -33,9 +34,12 @@
{
    private static final String PATH_SEPARATOR = "/";

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/test/java/org/apache/maven/ artifact/DefaultArtifactTest.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/test/java/org/apache/maven/artifact/DefaultArtifactTest.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java (original) +++ maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java Sat Feb 2 20:19:44 2008 
@@ -47,12 +47,11 @@
        throws Exception
    {
        super.setUp();
-        artifactHandler = new ArtifactHandlerMock();
        versionRange = VersionRange.createFromVersion( version );
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, classifier, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, classifier );
snapshotVersionRange = VersionRange.createFromVersion( snapshotResolvedVersion ); - snapshotArtifact = new DefaultArtifact( groupId, artifactId, snapshotVersionRange, scope, type, classifier, artifactHandler ); + snapshotArtifact = new DefaultArtifact( groupId, artifactId, snapshotVersionRange, scope, type, classifier ); 
    }

    public void testGetVersionReturnsResolvedVersionOnSnapshot()
@@ -78,7 +77,7 @@

    public void testGetDependencyConflictIdNullClassifier()
    {
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null ); assertEquals( groupId + ":" + artifactId + ":" + type, artifact.getDependencyConflictId() ); 
    }

@@ -102,7 +101,7 @@

    public void testToStringNullClassifier()
    {
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null ); assertEquals( groupId + ":" + artifactId + ":" + type + ":" + version + ":" + scope, artifact.toString() ); 
    }
Modified: maven/artifact/trunk/src/test/java/org/apache/maven/ artifact/manager/DefaultWagonManagerTest.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/test/java/org/apache/maven/artifact/manager/DefaultWagonManagerTest.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java (original) +++ maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java Sat Feb 2 20:19:44 2008 
@@ -138,7 +138,7 @@
        {
            tmpFile.deleteOnExit();
Artifact artifact = new DefaultArtifact( "sample.group", "sample-art", VersionRange - .createFromVersion( "1.0" ), "artifactScope", "type", "classifier", null ); + .createFromVersion( "1.0" ), "artifactScope", "type", "classifier" ); 
            artifact.setFile( tmpFile );
ArtifactRepository repo = new DefaultArtifactRepository( "id", "noop://url", new ArtifactRepositoryLayoutStub() ); 





---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



Thanks,

Jason

----------------------------------------------------------
Jason van Zyl
Founder,  Apache Maven
jason at sonatype dot com
----------------------------------------------------------

You are never dedicated to something you have complete confidence in.
No one is fanatically shouting that the sun is going to rise tomorrow.
They know it is going to rise tomorrow. When people are fanatically
dedicated to political or religious faiths or any other kind of
dogmas or goals, it's always because these dogmas or
goals are in doubt.
-- Robert Pirzig, Zen and the Art of Motorcycle Maintenance 



---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to