Hi, Yes I agree with you that I should have an adapter for each scm with the current adapter as a wrapper, the question I need to think about is how should I inject the correct adapter into the wrapper (through the constructor like the ScmManager and the grammar, or with a static map). Also I agree with you for the interface SvnGrammar and its descendants which should be renamed ScmGrammar etc. The same is to be applied to SvnTargetEnum and SvnLogEntry. Adding Hg was more a proof of concept than a real integration and it shows through these misnamings ;o) I will put this into JIRA. As it is cosmetic and add no functionnality I would rather refactor for the 1.2 version, pushing the current stable (as with not bug entry left) into production so current users can take advantage of the NPE correction and the filter parameter and I can get more feedbacks. Thanks for your remarks. Emmanuel
On Thu, Feb 5, 2009 at 12:09 PM, Mark Struberg <[email protected]> wrote: > Hi Emmanuel! > > > > In general, I would prefer to have Hg and Svn specific > > code only in their > > > own packages, using interfaces to access them. I > > personally don't think it's > > > a good idea to have getSvnXxxx and getHgXxxx functions > > in one file. > > > Otherwise I fear this will cause a problem in > > maintaining the code later if > > > other scm providers will be added. > > > > You are right, but the ScmManager doesn't provide such > > facility, maybe this > > should move to the provider. > > Sorry, I was not clear enough: I didn't mean to move this code over to the > ScmManager, but only to move those few bits which are still mixed > (getSvnListOfReleases, getHgListOfReleases) to the already existing packages > org.codehaus.mojo.scmchangelog.scm.hg and > org.codehaus.mojo.scmchangelog.scm.svn > and call them via interface or kind of SPI. > > LieGrue, > strub > > > --- Emmanuel Hugonnet <[email protected]> schrieb am Do, 5.2.2009: > > > Von: Emmanuel Hugonnet <[email protected]> > > Betreff: Re: [mojo-dev] [Vote] Scmchangelog-maven-plugin > > An: [email protected] > > Datum: Donnerstag, 5. Februar 2009, 11:54 > > Hi, > > > > On Thu, Feb 5, 2009 at 11:02 AM, Mark Struberg > > <[email protected]> wrote: > > > > > Hi! > > > > > > Sorry, but I'd give it a > > > -1 > > > because from only taking a glimpse at the xref, the > > code seems a bit mixed > > > up, e.g. in > > > > > > protected List ScmAdapter#getHgListOfReleases(.. > > > > > > 201 final ChangeLogScmResult logs = > > this.manager.changeLog( > > > repository, > > > 202 fileSet, getScmVersion( > > SvnTargetEnum.TRUNK, startRevision ), > > > 203 getScmVersion( SvnTargetEnum.TRUNK, > > tag.getEndRevision() ), > > > "" ); > > > > > > I personally don't like it much if constants for > > Svn are also used for > > > Mercurial, etc. So maybe, you can simply refactor out > > the real scm provider > > > independent constants (and functionality in general) > > and separate the > > > concerns better? > > > > > > Sorry, I think the Enum name came from the fact that > > historically this was a > > svn only plugin, When we migrated to hg and add potential > > support to other > > scms I did some refactoring but I may have missed some > > renaming. > > > > > > > > In general, I would prefer to have Hg and Svn specific > > code only in their > > > own packages, using interfaces to access them. I > > personally don't think it's > > > a good idea to have getSvnXxxx and getHgXxxx functions > > in one file. > > > Otherwise I fear this will cause a problem in > > maintaining the code later if > > > other scm providers will be added. > > > > You are right, but the ScmManager doesn't provide such > > facility, maybe this > > should move to the provider. > > > > > > > > > > > This brings me to the next very basic question: Maybe > > there is a > > > fundamental reason and you already explained it > > (didn't follow the project > > > closely), but is there a reason why you don't use > > the specialized > > > ChangeLogConsumers provided by all the > > maven-scm-providers? Is there any > > > important information missing in the ChangeLogSets > > returned by? Or is the > > > information inside the results that different? > > > If this information is really helpful or you see a > > chance to unify them, > > > you may talk with Jason, Olivier Lamy and Emanuel > > Venisse to extend the > > > scm-API? > > > > > > Yes there is also a reason that is explained in the for > > developpers section. > > First I was missing infos with the svn command if I > > didn't use xml (don't > > remember much of it but it was my problem then) and second > > I need a > > BetterChangeSet instead of the simple ChangeSet from hg > > where I can set the > > revision number for the changeset. This info is available > > for most current > > scms but it is not set by scm-api so I am losing some info > > there also. > > That is why I had to override the scm-api. > > Also I do som filtering in the hg tags consumer and > > SvnListConsumer which is > > not available from the scm-api. > > > > > > > > > > txs and LieGrue, > > > strub > > > > > > Emmanuel > > > > > > > > > > > --- Emmanuel Hugonnet <[email protected]> > > schrieb am Mi, 4.2.2009: > > > > > > > Von: Emmanuel Hugonnet > > <[email protected]> > > > > Betreff: [mojo-dev] [Vote] > > Scmchangelog-maven-plugin > > > > An: [email protected] > > > > Datum: Mittwoch, 4. Februar 2009, 15:54 > > > > Hi, > > > > > > > > We have been working on the > > scmchangelog-maven-plugin and > > > > we have closed all > > > > opened issues. > > > > > > > > The snapshot is available at > > > > * > > > > > > > > > > http://snapshots.repository.codehaus.org/org/codehaus/mojo/scmchangelog-maven-plugin/1.1-SNAPSHOT > > > > *< > > > > > > http://snapshots.repository.codehaus.org/org/codehaus/mojo/scmchangelog-maven-plugin/1.1-SNAPSHOT > > > > > > > > > > > > The new site is available at > > > > > > *http://mojo.codehaus.org/*scmchangelog-maven-plugin< > > > http://mojo.codehaus.org/scmchangelog-maven-plugin> > > > > > > > > The change log is available at > > > > > > *http://mojo.codehaus.org/scmchangelog-maven-plugin/changelog.html*< > > > http://mojo.codehaus.org/rpm-maven-plugin> > > > > > > > > This vote will be open for 72 hours and will use > > lazy > > > > consensus. > > > > [+1] release it > > > > [0] don't care > > > > [-1] don't release! > > > > > > > > Thanks, > > > > > > > > Rémy & Emmanuel > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > To unsubscribe from this list, please visit: > > > > > > http://xircles.codehaus.org/manage_email > > > > > > > > > > > > > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > >
