Hi Egon (and others) I'd go for patch 2. Classes are cheap to add, and less required commits means less possibility for human error.
also, arguing against patch 1: The version number is conceptually very different from what is currently in the CDKConstants interface. Right now it contains the "magic numbers" used internally _by_ the CDK implementation. The version number is meta-information _about_ the CDK. Conceptually different, so needs a different class. Furthermore, patch 1 would require a user to import the entire constants library for just the version, which bears some marks of beginning god-classes and the resulting dependency knot. (Having said that, the current mix of atom-, bond- and other property-labels in one class is also already a bit shady in this context) that's my 2 cents. Cheers, Jules On 22 July 2010 15:18, Egon Willighagen <[email protected]> wrote: > Hi all CDK-users, > > it is time for a short popularity poll: please do reply, whatevery > your opinion is. > > Rajarshi and I want to introduce a method to get the version number of > a particular CDK release. We both came up with two difference patches: > > Patch 1: > add a static string field to CDKConstants that gets updated by the > build script, but leads to one extra change and commit for each > version > > Patch 2: > use the version in the already existing build.props, and have a new > class with the CDK.getVersion() method. This has the obvious downside > that we have another extra class, but does not require code changes > with each new version. > > Both for pros and cons... please discuss your favorite approach, > further ideas, etc. > > One particular question would be: is there other information about the > library you like to have available via the API? > > Your input would be very much appreciated! > > Egon > > > ---------- Forwarded message ---------- > From: Rajarshi Guha <[email protected]> > Date: Thu, Jul 22, 2010 at 3:03 PM > Subject: Re: [Cdk-devel] mol2 reader not working > To: Egon Willighagen <[email protected]> > Cc: "Developers forum for discussion about the Chemistry Development > Kit (CDK)" <[email protected]> > > On Thu, Jul 22, 2010 at 2:45 AM, Rajarshi wrote: >>> The following we should take to the mailing list, I guess... >>> >>> 3018684 Add a constant to indicate library version >>> >>> Please check my comments, and see how I can make my patch acceptable >>> for you... would it help if I added more functionality to the CDK >> >> Your approach makes sense, but I still think a whole class extra class >> for a version string is a bit overkill. Lets see what the mailling >> ist has to say? > > -- > Rajarshi Guha > NIH Chemical Genomics Center > > > > -- > Post-doc @ Uppsala University > Proteochemometrics / Bioclipse Group of Prof. Jarl Wikberg > Homepage: http://egonw.github.com/ > Blog: http://chem-bla-ics.blogspot.com/ > PubList: http://www.citeulike.org/user/egonw/tag/papers > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by Sprint > What will you do first with EVO, the first 4G phone? > Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first > _______________________________________________ > Cdk-user mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/cdk-user > ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ Cdk-user mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/cdk-user
