subject:"svn commit\: r627675 \- in \/maven\/components\/branches\/maven\-2.0.x\/maven\-project\/src\: main\/java\/org\/apache\/maven\/project\/MavenProject.java test\/java\/org\/apache\/maven\/project\/MavenProjectTest.java"

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Brett Porter


Carlos,

Can you elaborate on the need for this?

I understand that since MavenProject is non-final and so are the get/ 
sets they can be overridden and so we should be using the get/set  
internally. However, it would seem we don't need that funcationality  
for every field - which particular ones do you see as needing to be  
overridden?


Also, I don't think the clone() stuff is right:
- you've deprecated the copy constructor even though it is still  
useful. You also require it's existence which means it shouldn't be  
deprecated.
- clone()'s contract says that it doesn't call any constructors,  
making the method work but not as documented by the JDK

- clone() should call super.clone() to get a valid MavenProject instance
- MavenProject doesn't implement clonable
Why did you need clone()?

Thanks,
Brett

On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:


Author: carlos
Date: Wed Feb 13 22:40:35 2008
New Revision: 627675

URL: http://svn.apache.org/viewvc?rev=627675view=rev
Log:
[MNG-3400] MavenProject is not extensible. Merge rev 627670 from trunk

Modified:
   maven/components/branches/maven-2.0.x/maven-project/src/main/java/ 
org/apache/maven/project/MavenProject.java
   maven/components/branches/maven-2.0.x/maven-project/src/test/java/ 
org/apache/maven/project/MavenProjectTest.java


Modified: maven/components/branches/maven-2.0.x/maven-project/src/ 
main/java/org/apache/maven/project/MavenProject.java

URL: 
http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven-project/src/main/java/org/apache/maven/project/MavenProject.java?rev=627675r1=627674r2=627675view=diff
= 
= 
= 
= 
= 
= 
= 
= 
==
--- maven/components/branches/maven-2.0.x/maven-project/src/main/ 
java/org/apache/maven/project/MavenProject.java (original)
+++ maven/components/branches/maven-2.0.x/maven-project/src/main/ 
java/org/apache/maven/project/MavenProject.java Wed Feb 13 22:40:35  
2008

@@ -158,103 +158,107 @@
model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
model.setVersion( EMPTY_PROJECT_VERSION );

-this.model = model;
+this.setModel( model );
}

public MavenProject( Model model )
{
-this.model = model;
+this.setModel( model );
}

+/**
+ * @deprecated use [EMAIL PROTECTED] #clone()}
+ */
public MavenProject( MavenProject project )
{
// disown the parent

// copy fields
-this.file = project.file;
+setFile( project.getFile() );

-// don't need a deep copy, they don't get modified or added/ 
removed to/from - but make them unmodifiable to be sure!

-if ( project.dependencyArtifacts != null )
+// don't need a deep copy, they don't get modified or added/ 
removed to/from - but make them unmodifiable to be

+// sure!
+if ( project.getDependencyArtifacts() != null )
{
-this.dependencyArtifacts =  
Collections.unmodifiableSet( project.dependencyArtifacts );
+ 
setDependencyArtifacts 
( Collections.unmodifiableSet( project.getDependencyArtifacts() ) );

}
-
-if ( project.artifacts != null )
+
+if ( project.getArtifacts() != null )
{
-this.artifacts =  
Collections.unmodifiableSet( project.artifacts );
+ 
setArtifacts( Collections.unmodifiableSet( project.getArtifacts() ) );

}
-
-if ( project.pluginArtifacts != null )
+
+if ( project.getPluginArtifacts() != null )
{
-this.pluginArtifacts =  
Collections.unmodifiableSet( project.pluginArtifacts );
+ 
setPluginArtifacts 
( Collections.unmodifiableSet( project.getPluginArtifacts() ) );

}
-
-if ( project.reportArtifacts != null )
+
+if ( project.getReportArtifacts() != null )
{
-this.reportArtifacts =  
Collections.unmodifiableSet( project.reportArtifacts );

-}
-
-if ( project.extensionArtifacts != null )
+ 
setReportArtifacts 
( Collections.unmodifiableSet( project.getReportArtifacts() ) );

+}
+
+if ( project.getExtensionArtifacts() != null )
{
-this.extensionArtifacts =  
Collections.unmodifiableSet( project.extensionArtifacts );

-}
-
-this.parentArtifact = project.parentArtifact;
+ 
setExtensionArtifacts 
( Collections.unmodifiableSet( project.getExtensionArtifacts() ) );

+}
+
+setParentArtifact( ( project.getParentArtifact() ) );

-if ( project.remoteArtifactRepositories != null )
+if ( project.getRemoteArtifactRepositories() != null )
{
-this.remoteArtifactRepositories =  
Collections.unmodifiableList( project.remoteArtifactRepositories );

-}
-
-if ( project.pluginArtifactRepositories != null )
+ 
setRemoteArtifactRepositories 
( Collections 
.unmodifiableList(

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Carlos Sanchez

Reading carefully the javadoc I see the mistakes i made in clone :( I
can fix that

The problem arised with a delegate pattern implementation, the
MavenProject instance is encapsulated [1], but the problem comes with
other classes using this constructor to make copies, which will ignore
any customizations made in the delegating object (the subclass).

If the way to make a copy where defined in a method ( clone() seems to
be the right one ) then subclasses could just override that method and
there wouldnt be any need of getters/setters, but right now that
constructor is used in the maven archiver. Adding the getters and
setters is a patch until the other classes are updated to use clone()
and to keep backwards compatibility.

[1] http://tinyurl.com/29jzte


On Thu, Feb 14, 2008 at 4:15 PM, Brett Porter [EMAIL PROTECTED] wrote:
 Carlos,

  Can you elaborate on the need for this?

  I understand that since MavenProject is non-final and so are the get/
  sets they can be overridden and so we should be using the get/set
  internally. However, it would seem we don't need that funcationality
  for every field - which particular ones do you see as needing to be
  overridden?

  Also, I don't think the clone() stuff is right:
  - you've deprecated the copy constructor even though it is still
  useful. You also require it's existence which means it shouldn't be
  deprecated.
  - clone()'s contract says that it doesn't call any constructors,
  making the method work but not as documented by the JDK
  - clone() should call super.clone() to get a valid MavenProject instance
  - MavenProject doesn't implement clonable
  Why did you need clone()?

  Thanks,
  Brett

  On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:

   Author: carlos
   Date: Wed Feb 13 22:40:35 2008
   New Revision: 627675
  
   URL: http://svn.apache.org/viewvc?rev=627675view=rev
   Log:
   [MNG-3400] MavenProject is not extensible. Merge rev 627670 from trunk
  
   Modified:
  maven/components/branches/maven-2.0.x/maven-project/src/main/java/
   org/apache/maven/project/MavenProject.java
  maven/components/branches/maven-2.0.x/maven-project/src/test/java/
   org/apache/maven/project/MavenProjectTest.java
  
   Modified: maven/components/branches/maven-2.0.x/maven-project/src/
   main/java/org/apache/maven/project/MavenProject.java
   URL: 
 http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven-project/src/main/java/org/apache/maven/project/MavenProject.java?rev=627675r1=627674r2=627675view=diff
   =
   =
   =
   =
   =
   =
   =
   =
   ==
   --- maven/components/branches/maven-2.0.x/maven-project/src/main/
   java/org/apache/maven/project/MavenProject.java (original)
   +++ maven/components/branches/maven-2.0.x/maven-project/src/main/
   java/org/apache/maven/project/MavenProject.java Wed Feb 13 22:40:35
   2008
   @@ -158,103 +158,107 @@
   model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
   model.setVersion( EMPTY_PROJECT_VERSION );
  
   -this.model = model;
   +this.setModel( model );
   }
  
   public MavenProject( Model model )
   {
   -this.model = model;
   +this.setModel( model );
   }
  
   +/**
   + * @deprecated use [EMAIL PROTECTED] #clone()}
   + */
   public MavenProject( MavenProject project )
   {
   // disown the parent
  
   // copy fields
   -this.file = project.file;
   +setFile( project.getFile() );
  
   -// don't need a deep copy, they don't get modified or added/
   removed to/from - but make them unmodifiable to be sure!
   -if ( project.dependencyArtifacts != null )
   +// don't need a deep copy, they don't get modified or added/
   removed to/from - but make them unmodifiable to be
   +// sure!
   +if ( project.getDependencyArtifacts() != null )
   {
   -this.dependencyArtifacts =
   Collections.unmodifiableSet( project.dependencyArtifacts );
   +
   setDependencyArtifacts
   ( Collections.unmodifiableSet( project.getDependencyArtifacts() ) );
   }
   -
   -if ( project.artifacts != null )
   +
   +if ( project.getArtifacts() != null )
   {
   -this.artifacts =
   Collections.unmodifiableSet( project.artifacts );
   +
   setArtifacts( Collections.unmodifiableSet( project.getArtifacts() ) );
   }
   -
   -if ( project.pluginArtifacts != null )
   +
   +if ( project.getPluginArtifacts() != null )
   {
   -this.pluginArtifacts =
   Collections.unmodifiableSet( project.pluginArtifacts );
   +
   setPluginArtifacts
   ( Collections.unmodifiableSet( project.getPluginArtifacts() ) );
   }
   -
   -if ( project.reportArtifacts != null )
   +
   +if ( project.getReportArtifacts() != null )
   {
   -this.reportArtifacts =

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Brett Porter

I'm not sure why the archiver needs to use the delegate? Is it because  
you lose track of the updates? IF that's all, then you could follow  
the @todo in the class javadoc :)


If it truly needs it, clone is the right method - I'd definitely  
recommend reading the section on clone in Effective Java though :)


- Brett

On 15/02/2008, at 11:57 AM, Carlos Sanchez wrote:


Reading carefully the javadoc I see the mistakes i made in clone :( I
can fix that

The problem arised with a delegate pattern implementation, the
MavenProject instance is encapsulated [1], but the problem comes with
other classes using this constructor to make copies, which will ignore
any customizations made in the delegating object (the subclass).

If the way to make a copy where defined in a method ( clone() seems to
be the right one ) then subclasses could just override that method and
there wouldnt be any need of getters/setters, but right now that
constructor is used in the maven archiver. Adding the getters and
setters is a patch until the other classes are updated to use clone()
and to keep backwards compatibility.

[1] http://tinyurl.com/29jzte


On Thu, Feb 14, 2008 at 4:15 PM, Brett Porter [EMAIL PROTECTED]  
wrote:

Carlos,

Can you elaborate on the need for this?

I understand that since MavenProject is non-final and so are the get/
sets they can be overridden and so we should be using the get/set
internally. However, it would seem we don't need that funcationality
for every field - which particular ones do you see as needing to be
overridden?

Also, I don't think the clone() stuff is right:
- you've deprecated the copy constructor even though it is still
useful. You also require it's existence which means it shouldn't be
deprecated.
- clone()'s contract says that it doesn't call any constructors,
making the method work but not as documented by the JDK
- clone() should call super.clone() to get a valid MavenProject  
instance

- MavenProject doesn't implement clonable
Why did you need clone()?

Thanks,
Brett

On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:


Author: carlos
Date: Wed Feb 13 22:40:35 2008
New Revision: 627675

URL: http://svn.apache.org/viewvc?rev=627675view=rev
Log:
[MNG-3400] MavenProject is not extensible. Merge rev 627670 from  
trunk


Modified:
  maven/components/branches/maven-2.0.x/maven-project/src/main/java/
org/apache/maven/project/MavenProject.java
  maven/components/branches/maven-2.0.x/maven-project/src/test/java/
org/apache/maven/project/MavenProjectTest.java

Modified: maven/components/branches/maven-2.0.x/maven-project/src/
main/java/org/apache/maven/project/MavenProject.java
URL: 
http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven-project/src/main/java/org/apache/maven/project/MavenProject.java?rev=627675r1=627674r2=627675view=diff
=
=
=
=
=
=
=
=
= 
= 


--- maven/components/branches/maven-2.0.x/maven-project/src/main/
java/org/apache/maven/project/MavenProject.java (original)
+++ maven/components/branches/maven-2.0.x/maven-project/src/main/
java/org/apache/maven/project/MavenProject.java Wed Feb 13 22:40:35
2008
@@ -158,103 +158,107 @@
   model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
   model.setVersion( EMPTY_PROJECT_VERSION );

-this.model = model;
+this.setModel( model );
   }

   public MavenProject( Model model )
   {
-this.model = model;
+this.setModel( model );
   }

+/**
+ * @deprecated use [EMAIL PROTECTED] #clone()}
+ */
   public MavenProject( MavenProject project )
   {
   // disown the parent

   // copy fields
-this.file = project.file;
+setFile( project.getFile() );

-// don't need a deep copy, they don't get modified or  
added/

removed to/from - but make them unmodifiable to be sure!
-if ( project.dependencyArtifacts != null )
+// don't need a deep copy, they don't get modified or  
added/

removed to/from - but make them unmodifiable to be
+// sure!
+if ( project.getDependencyArtifacts() != null )
   {
-this.dependencyArtifacts =
Collections.unmodifiableSet( project.dependencyArtifacts );
+
setDependencyArtifacts
( Collections.unmodifiableSet( project.getDependencyArtifacts() ) );
   }
-
-if ( project.artifacts != null )
+
+if ( project.getArtifacts() != null )
   {
-this.artifacts =
Collections.unmodifiableSet( project.artifacts );
+
setArtifacts 
( Collections.unmodifiableSet( project.getArtifacts() ) );

   }
-
-if ( project.pluginArtifacts != null )
+
+if ( project.getPluginArtifacts() != null )
   {
-this.pluginArtifacts =
Collections.unmodifiableSet( project.pluginArtifacts );
+
setPluginArtifacts
( Collections.unmodifiableSet( project.getPluginArtifacts() ) );
   }
-
-if ( project.reportArtifacts != null )
+
+if ( project.getReportArtifacts() !=

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Jason van Zyl



On 14-Feb-08, at 4:57 PM, Carlos Sanchez wrote:


Reading carefully the javadoc I see the mistakes i made in clone :( I
can fix that

The problem arised with a delegate pattern implementation, the
MavenProject instance is encapsulated [1], but the problem comes with
other classes using this constructor to make copies, which will ignore
any customizations made in the delegating object (the subclass).

If the way to make a copy where defined in a method ( clone() seems to
be the right one ) then subclasses could just override that method and
there wouldnt be any need of getters/setters, but right now that
constructor is used in the maven archiver.


In the Archiver?? That just sounds wrong. It should be more of  
extracting the information necessary from the POM and passing that in  
as a set of attributes to be used by the Archiver.



Adding the getters and
setters is a patch until the other classes are updated to use clone()
and to keep backwards compatibility.

[1] http://tinyurl.com/29jzte


On Thu, Feb 14, 2008 at 4:15 PM, Brett Porter [EMAIL PROTECTED]  
wrote:

Carlos,

Can you elaborate on the need for this?

I understand that since MavenProject is non-final and so are the get/
sets they can be overridden and so we should be using the get/set
internally. However, it would seem we don't need that funcationality
for every field - which particular ones do you see as needing to be
overridden?

Also, I don't think the clone() stuff is right:
- you've deprecated the copy constructor even though it is still
useful. You also require it's existence which means it shouldn't be
deprecated.
- clone()'s contract says that it doesn't call any constructors,
making the method work but not as documented by the JDK
- clone() should call super.clone() to get a valid MavenProject  
instance

- MavenProject doesn't implement clonable
Why did you need clone()?

Thanks,
Brett

On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:


Author: carlos
Date: Wed Feb 13 22:40:35 2008
New Revision: 627675

URL: http://svn.apache.org/viewvc?rev=627675view=rev
Log:
[MNG-3400] MavenProject is not extensible. Merge rev 627670 from  
trunk


Modified:
  maven/components/branches/maven-2.0.x/maven-project/src/main/java/
org/apache/maven/project/MavenProject.java
  maven/components/branches/maven-2.0.x/maven-project/src/test/java/
org/apache/maven/project/MavenProjectTest.java

Modified: maven/components/branches/maven-2.0.x/maven-project/src/
main/java/org/apache/maven/project/MavenProject.java
URL: 
http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven-project/src/main/java/org/apache/maven/project/MavenProject.java?rev=627675r1=627674r2=627675view=diff
=
=
=
=
=
=
=
=
= 
= 


--- maven/components/branches/maven-2.0.x/maven-project/src/main/
java/org/apache/maven/project/MavenProject.java (original)
+++ maven/components/branches/maven-2.0.x/maven-project/src/main/
java/org/apache/maven/project/MavenProject.java Wed Feb 13 22:40:35
2008
@@ -158,103 +158,107 @@
   model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
   model.setVersion( EMPTY_PROJECT_VERSION );

-this.model = model;
+this.setModel( model );
   }

   public MavenProject( Model model )
   {
-this.model = model;
+this.setModel( model );
   }

+/**
+ * @deprecated use [EMAIL PROTECTED] #clone()}
+ */
   public MavenProject( MavenProject project )
   {
   // disown the parent

   // copy fields
-this.file = project.file;
+setFile( project.getFile() );

-// don't need a deep copy, they don't get modified or  
added/

removed to/from - but make them unmodifiable to be sure!
-if ( project.dependencyArtifacts != null )
+// don't need a deep copy, they don't get modified or  
added/

removed to/from - but make them unmodifiable to be
+// sure!
+if ( project.getDependencyArtifacts() != null )
   {
-this.dependencyArtifacts =
Collections.unmodifiableSet( project.dependencyArtifacts );
+
setDependencyArtifacts
( Collections.unmodifiableSet( project.getDependencyArtifacts() ) );
   }
-
-if ( project.artifacts != null )
+
+if ( project.getArtifacts() != null )
   {
-this.artifacts =
Collections.unmodifiableSet( project.artifacts );
+
setArtifacts 
( Collections.unmodifiableSet( project.getArtifacts() ) );

   }
-
-if ( project.pluginArtifacts != null )
+
+if ( project.getPluginArtifacts() != null )
   {
-this.pluginArtifacts =
Collections.unmodifiableSet( project.pluginArtifacts );
+
setPluginArtifacts
( Collections.unmodifiableSet( project.getPluginArtifacts() ) );
   }
-
-if ( project.reportArtifacts != null )
+
+if ( project.getReportArtifacts() != null )
   {
-this.reportArtifacts =
Collections.unmodifiableSet( project.reportArtifacts );
-}
-
-if

RE: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Brian E. Fox

It seems curious to me that the archiver needs to understand eclipse.
Isn't this a generic component? Perhaps you should be making
maven-eclipse-archiver or something.

-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Carlos
Sanchez
Sent: Thursday, February 14, 2008 5:41 PM
To: Maven Developers List
Subject: Re: svn commit: r627675 - in
/maven/components/branches/maven-2.0.x/maven-project/src:
main/java/org/apache/maven/project/MavenProject.java
test/java/org/apache/maven/project/MavenProjectTest.java

The archiver is making a copy of the MavenProject using newProject =
new MavenProject(project)
project is a subclass of MavenProject (EclipseMavenProject)
Instead the archiver should do project.clone() if any

Previous to my patch new MavenProject(project) fails with NPE as the
fields are accessed directly. After the patch it works, although I
still think to make copies it should use clone() and that's why I
deprecated the constructor

what @todo are you talking about?

I will fix the clone method.

On Thu, Feb 14, 2008 at 5:17 PM, Brett Porter [EMAIL PROTECTED] wrote:
 I'm not sure why the archiver needs to use the delegate? Is it because
  you lose track of the updates? IF that's all, then you could follow
  the @todo in the class javadoc :)

  If it truly needs it, clone is the right method - I'd definitely
  recommend reading the section on clone in Effective Java though :)

  - Brett

  On 15/02/2008, at 11:57 AM, Carlos Sanchez wrote:

   Reading carefully the javadoc I see the mistakes i made in clone :(
I
   can fix that

   The problem arised with a delegate pattern implementation, the
   MavenProject instance is encapsulated [1], but the problem comes
with
   other classes using this constructor to make copies, which will
ignore
   any customizations made in the delegating object (the subclass).

   If the way to make a copy where defined in a method ( clone() seems
to
   be the right one ) then subclasses could just override that method
and
   there wouldnt be any need of getters/setters, but right now that
   constructor is used in the maven archiver. Adding the getters and
   setters is a patch until the other classes are updated to use
clone()
   and to keep backwards compatibility.

   [1] http://tinyurl.com/29jzte

   On Thu, Feb 14, 2008 at 4:15 PM, Brett Porter [EMAIL PROTECTED]
   wrote:
   Carlos,

   Can you elaborate on the need for this?

   I understand that since MavenProject is non-final and so are the
get/
   sets they can be overridden and so we should be using the get/set
   internally. However, it would seem we don't need that
funcationality
   for every field - which particular ones do you see as needing to
be
   overridden?

   Also, I don't think the clone() stuff is right:
   - you've deprecated the copy constructor even though it is still
   useful. You also require it's existence which means it shouldn't
be
   deprecated.
   - clone()'s contract says that it doesn't call any constructors,
   making the method work but not as documented by the JDK
   - clone() should call super.clone() to get a valid MavenProject
   instance
   - MavenProject doesn't implement clonable
   Why did you need clone()?

   Thanks,
   Brett

   On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:

   Author: carlos
   Date: Wed Feb 13 22:40:35 2008
   New Revision: 627675

   URL: http://svn.apache.org/viewvc?rev=627675view=rev
   Log:
   [MNG-3400] MavenProject is not extensible. Merge rev 627670 from
   trunk

   Modified:

maven/components/branches/maven-2.0.x/maven-project/src/main/java/
   org/apache/maven/project/MavenProject.java

maven/components/branches/maven-2.0.x/maven-project/src/test/java/
   org/apache/maven/project/MavenProjectTest.java

   Modified:
maven/components/branches/maven-2.0.x/maven-project/src/
   main/java/org/apache/maven/project/MavenProject.java
   URL:
http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven
-project/src/main/java/org/apache/maven/project/MavenProject.java?rev=62
7675r1=627674r2=627675view=diff
   =
   =
   =
   =
   =
   =
   =
   =
   =
   =

   --- maven/components/branches/maven-2.0.x/maven-project/src/main/
   java/org/apache/maven/project/MavenProject.java (original)
   +++ maven/components/branches/maven-2.0.x/maven-project/src/main/
   java/org/apache/maven/project/MavenProject.java Wed Feb 13
22:40:35
   2008
   @@ -158,103 +158,107 @@
  model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
  model.setVersion( EMPTY_PROJECT_VERSION );

   -this.model = model;
   +this.setModel( model );
  }

  public MavenProject( Model model )
  {
   -this.model = model;
   +this.setModel( model );
  }

   +/**
   + * @deprecated use [EMAIL PROTECTED] #clone()}
   + */
  public MavenProject( MavenProject project

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Brett Porter



On 15/02/2008, at 12:41 PM, Carlos Sanchez wrote:


The archiver is making a copy of the MavenProject using newProject =
new MavenProject(project)
project is a subclass of MavenProject (EclipseMavenProject)
Instead the archiver should do project.clone() if any


I think I was asking the same thing as Jason - I didn't know why the  
archiver should be creating a new project (and if it was, why it would  
need to be another instance of the delegate class).




what @todo are you talking about?


I meant in the other code, but it's not relevant since you aren't  
doing this for lack of update visibility.


- Brett

--
Brett Porter
[EMAIL PROTECTED]
http://blogs.exist.com/bporter/


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

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Carlos Sanchez

The archiver doesnt have to understand eclipse. But it can't create a
MavenProject object when the one passed is a subclass that may have
customized behavior

On Thu, Feb 14, 2008 at 5:42 PM, Brian E. Fox [EMAIL PROTECTED] wrote:
 It seems curious to me that the archiver needs to understand eclipse.
  Isn't this a generic component? Perhaps you should be making
  maven-eclipse-archiver or something.


  -Original Message-
  From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Carlos
  Sanchez
  Sent: Thursday, February 14, 2008 5:41 PM
  To: Maven Developers List
  Subject: Re: svn commit: r627675 - in
  /maven/components/branches/maven-2.0.x/maven-project/src:
  main/java/org/apache/maven/project/MavenProject.java


 test/java/org/apache/maven/project/MavenProjectTest.java

  The archiver is making a copy of the MavenProject using newProject =
  new MavenProject(project)
  project is a subclass of MavenProject (EclipseMavenProject)
  Instead the archiver should do project.clone() if any

  Previous to my patch new MavenProject(project) fails with NPE as the
  fields are accessed directly. After the patch it works, although I
  still think to make copies it should use clone() and that's why I
  deprecated the constructor

  what @todo are you talking about?

  I will fix the clone method.

  On Thu, Feb 14, 2008 at 5:17 PM, Brett Porter [EMAIL PROTECTED] wrote:
   I'm not sure why the archiver needs to use the delegate? Is it because
you lose track of the updates? IF that's all, then you could follow
the @todo in the class javadoc :)
  
If it truly needs it, clone is the right method - I'd definitely
recommend reading the section on clone in Effective Java though :)
  
- Brett
  
  
  
On 15/02/2008, at 11:57 AM, Carlos Sanchez wrote:
  
 Reading carefully the javadoc I see the mistakes i made in clone :(
  I
 can fix that

 The problem arised with a delegate pattern implementation, the
 MavenProject instance is encapsulated [1], but the problem comes
  with
 other classes using this constructor to make copies, which will
  ignore
 any customizations made in the delegating object (the subclass).

 If the way to make a copy where defined in a method ( clone() seems
  to
 be the right one ) then subclasses could just override that method
  and
 there wouldnt be any need of getters/setters, but right now that
 constructor is used in the maven archiver. Adding the getters and
 setters is a patch until the other classes are updated to use
  clone()
 and to keep backwards compatibility.

 [1] http://tinyurl.com/29jzte


 On Thu, Feb 14, 2008 at 4:15 PM, Brett Porter [EMAIL PROTECTED]
 wrote:
 Carlos,

 Can you elaborate on the need for this?

 I understand that since MavenProject is non-final and so are the
  get/
 sets they can be overridden and so we should be using the get/set
 internally. However, it would seem we don't need that
  funcationality
 for every field - which particular ones do you see as needing to
  be
 overridden?

 Also, I don't think the clone() stuff is right:
 - you've deprecated the copy constructor even though it is still
 useful. You also require it's existence which means it shouldn't
  be
 deprecated.
 - clone()'s contract says that it doesn't call any constructors,
 making the method work but not as documented by the JDK
 - clone() should call super.clone() to get a valid MavenProject
 instance
 - MavenProject doesn't implement clonable
 Why did you need clone()?

 Thanks,
 Brett

 On 14/02/2008, at 5:40 PM, [EMAIL PROTECTED] wrote:

 Author: carlos
 Date: Wed Feb 13 22:40:35 2008
 New Revision: 627675

 URL: http://svn.apache.org/viewvc?rev=627675view=rev
 Log:
 [MNG-3400] MavenProject is not extensible. Merge rev 627670 from
 trunk

 Modified:

  maven/components/branches/maven-2.0.x/maven-project/src/main/java/
 org/apache/maven/project/MavenProject.java

  maven/components/branches/maven-2.0.x/maven-project/src/test/java/
 org/apache/maven/project/MavenProjectTest.java

 Modified:
  maven/components/branches/maven-2.0.x/maven-project/src/
 main/java/org/apache/maven/project/MavenProject.java
 URL:
  http://svn.apache.org/viewvc/maven/components/branches/maven-2.0.x/maven
  -project/src/main/java/org/apache/maven/project/MavenProject.java?rev=62
  7675r1=627674r2=627675view=diff
 =
 =
 =
 =
 =
 =
 =
 =
 =
 =

  
 --- maven/components/branches/maven-2.0.x/maven-project/src/main/
 java/org/apache/maven/project/MavenProject.java (original)
 +++ maven/components/branches/maven-2.0.x/maven-project/src/main/
 java/org/apache/maven/project/MavenProject.java Wed Feb 13

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Carlos Sanchez

I dont know why it is there, but it is, line 314
https://svn.apache.org/viewvc/maven/shared/trunk/maven-archiver/src/main/java/org/apache/maven/archiver/MavenArchiver.java?annotate=627672

emmanuel comment is
we have to clone the project instance so we can write out the pom with
the deployment version, without impacting the main project instance...


On Thu, Feb 14, 2008 at 5:51 PM, Brett Porter [EMAIL PROTECTED] wrote:

  On 15/02/2008, at 12:41 PM, Carlos Sanchez wrote:

   The archiver is making a copy of the MavenProject using newProject =
   new MavenProject(project)
   project is a subclass of MavenProject (EclipseMavenProject)
   Instead the archiver should do project.clone() if any

  I think I was asking the same thing as Jason - I didn't know why the
  archiver should be creating a new project (and if it was, why it would
  need to be another instance of the delegate class).


  
   what @todo are you talking about?

  I meant in the other code, but it's not relevant since you aren't
  doing this for lack of update visibility.

  - Brett



  --
  Brett Porter
  [EMAIL PROTECTED]
  http://blogs.exist.com/bporter/


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





-- 
I could give you my word as a Spaniard.
No good. I've known too many Spaniards.
 -- The Princess Bride

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

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Brett Porter

Ok, well in that case you don't need clone, you just needed to flip  
all the get/set's like you did and continue using the copy constructor.


On 15/02/2008, at 12:59 PM, Carlos Sanchez wrote:


I dont know why it is there, but it is, line 314
https://svn.apache.org/viewvc/maven/shared/trunk/maven-archiver/src/main/java/org/apache/maven/archiver/MavenArchiver.java?annotate=627672

emmanuel comment is
we have to clone the project instance so we can write out the pom with
the deployment version, without impacting the main project instance...


On Thu, Feb 14, 2008 at 5:51 PM, Brett Porter [EMAIL PROTECTED]  
wrote:


On 15/02/2008, at 12:41 PM, Carlos Sanchez wrote:


The archiver is making a copy of the MavenProject using newProject =
new MavenProject(project)
project is a subclass of MavenProject (EclipseMavenProject)
Instead the archiver should do project.clone() if any


I think I was asking the same thing as Jason - I didn't know why the
archiver should be creating a new project (and if it was, why it  
would

need to be another instance of the delegate class).




what @todo are you talking about?


I meant in the other code, but it's not relevant since you aren't
doing this for lack of update visibility.

- Brett



--
Brett Porter
[EMAIL PROTECTED]
http://blogs.exist.com/bporter/


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






--
I could give you my word as a Spaniard.
No good. I've known too many Spaniards.
-- The Princess Bride

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



--
Brett Porter
[EMAIL PROTECTED]
http://blogs.exist.com/bporter/


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

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

2008-02-14 Thread Carlos Sanchez

does this look better?
https://svn.apache.org/viewvc/maven/components/trunk/maven-project/src/main/java/org/apache/maven/project/MavenProject.java?r1=627670r2=627932pathrev=627932diff_format=h

On Thu, Feb 14, 2008 at 6:11 PM, Brett Porter [EMAIL PROTECTED] wrote:
 Ok, well in that case you don't need clone, you just needed to flip
  all the get/set's like you did and continue using the copy constructor.



  On 15/02/2008, at 12:59 PM, Carlos Sanchez wrote:

   I dont know why it is there, but it is, line 314
   
 https://svn.apache.org/viewvc/maven/shared/trunk/maven-archiver/src/main/java/org/apache/maven/archiver/MavenArchiver.java?annotate=627672
  
   emmanuel comment is
   we have to clone the project instance so we can write out the pom with
   the deployment version, without impacting the main project instance...
  
  
   On Thu, Feb 14, 2008 at 5:51 PM, Brett Porter [EMAIL PROTECTED]
   wrote:
  
   On 15/02/2008, at 12:41 PM, Carlos Sanchez wrote:
  
   The archiver is making a copy of the MavenProject using newProject =
   new MavenProject(project)
   project is a subclass of MavenProject (EclipseMavenProject)
   Instead the archiver should do project.clone() if any
  
   I think I was asking the same thing as Jason - I didn't know why the
   archiver should be creating a new project (and if it was, why it
   would
   need to be another instance of the delegate class).
  
  
  
   what @todo are you talking about?
  
   I meant in the other code, but it's not relevant since you aren't
   doing this for lack of update visibility.
  
   - Brett
  
  
  
   --
   Brett Porter
   [EMAIL PROTECTED]
   http://blogs.exist.com/bporter/
  
  
   -
   To unsubscribe, e-mail: [EMAIL PROTECTED]
   For additional commands, e-mail: [EMAIL PROTECTED]
  
  
  
  
  
   --
   I could give you my word as a Spaniard.
   No good. I've known too many Spaniards.
   -- The Princess Bride
  
   -
   To unsubscribe, e-mail: [EMAIL PROTECTED]
   For additional commands, e-mail: [EMAIL PROTECTED]
  

  --
  Brett Porter
  [EMAIL PROTECTED]
  http://blogs.exist.com/bporter/


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





-- 
I could give you my word as a Spaniard.
No good. I've known too many Spaniards.
 -- The Princess Bride

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

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

RE: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

Re: svn commit: r627675 - in /maven/components/branches/maven-2.0.x/maven-project/src: main/java/org/apache/maven/project/MavenProject.java test/java/org/apache/maven/project/MavenProjectTest.java

10 matches

Site Navigation

Mail list logo

Footer information