On Jul 11, 2008, at 2:38 PM, Dennis Lundberg wrote:
John Casey wrote:
Looking at the Clirr violations, it seems that the problem is a
change in the number of parameters to various method in the
generated parser classes:
[ERROR]
org.apache.maven.artifact.repository.metadata.io.xpp3.MetadataXpp3Rea
der: In method 'public boolean getBooleanValue(java.lang.String,
java.lang.String,
org.codehaus.plexus.util.xml.pull.XmlPullParser)' the number of
arguments has changed
[ERROR]
org.apache.maven.artifact.repository.metadata.io.xpp3.MetadataXpp3Rea
der: In method 'public java.util.Date getDateValue
(java.lang.String, java.lang.String,
org.codehaus.plexus.util.xml.pull.XmlPullParser)' the number of
arguments has changed
IMO, these methods are meant to be private, and shouldn't show up
at all in Clirr checks. Using these methods on the generated
parser classes is an abuse of the spirit of that class (i.e.
parsing a model, not value coercion), so there shouldn't be any
reason for these to need to be public...unless I'm missing something.
But how does a user of our artifacts know what the "spirit of that
class" is, unless we have that written down somewhere? I agree that
the methods should never have been made public in the first place,
but now they are. So in order to stay compatible with the current
API contract we should fix the issue rather than ignore it.
I would think that the name - MetadataXpp3Reader - would tip third-
party developers off to the fact that this is a parser, not a
conversion utility class. I understand that we have a problem with
what's public API and what's merely got a public modifier on its
class declaration, but it's simply not practical to think that we can
maintain the exact signature for every public interface in all of
Maven. I've already had to add methods to ArtifactMetadataSource and
MavenProjectBuilder in order to address some fairly serious bugs for
this release, not to mention a change to the (newly introduced in
2.0.9) ProjectBuilderConfiguration interface, to support the new
interpolation mechanism.
I think if we're going to try to take a hard line on maintaining a
public API, then we need to define that API in a separate artifact
that we can place strict limits on.
Since you mention adding a flag to Modello in the future (beyond -
alpha-19, I'm guessing) to allow us to set these sorts of methods
to private,
Yes, but we can't use that flag for the 2.0.x branch, since it
would break the compatibility with previous releases. That flag
would be for use in trunk, i.e. the coming 2.1 releases.
I guess I'm confused as to what you're planning to add to -
alpha-19 to fix the above generated compatibility problems.
This has already been fixed [4] in alpha-19 by adding the missing
methods, i.e. the one's with fewer parameters, making it fully
backwards compatible.
So, the new methods are now generated alongside the old, single-
parameter methods? In my experience with Clirr, this will still
trigger a build failure, since it considers methods *added* to the
interface as errors as well as those whose signatures change. I've
had to add exclusions for ArtifactMetadataSource and
MavenProjectBuilder along with others because of this.
I suppose it's worth mentioning that the definition of backwards
compatibility depends on whether you're implementing an interface or
merely using it. If you're only using it, then it's reasonable to
reinstate methods that people should reasonably be using, without
worrying about new methods. For outside implementations, this makes
little difference.
IMO, until we can give developers a Maven API that is blessed as
public, we're going to have these sorts of problems. I'm not sure
what the value of protecting compat with these particular methods is,
though.
[4] http://jira.codehaus.org/browse/MODELLO-111
---
John Casey
Committer and PMC Member, Apache Maven
mail: jdcasey at commonjava dot org
blog: http://www.ejlife.net/blogs/john
rss: http://feeds.feedburner.com/ejlife/john
