Re: [Cdk-devel] changes to cdk-fingerprinter

[Egon W. <eg...@sc...>]

On Saturday 31 January 2004 08:18, Rajarshi Guha wrote:
>   I was trying out cdk-fingerprinter and it failed whenever I fed it a
> HIN file or an XYZ file saying that the read function would only take
> ChemFile objects. Looking through the code for the format readers I see
> that this the exception  thrown. Since I based the HIN reading code of
> the XYZ reading code I assume that it is OK to throw an exception if the
> function recives something other than a ChemFile object.
>
> As a result I've edited applications/Fingerprinter.java so that it
> passes a ChemFile object to the read function. In addition I modified it
> to process multiple molecules if present in a given file.

That sounds like good changes.

> What is the protocol for these types of changes (i.e., changes to
> someone elses code)? Do I go ahead and directly submit it to CVS? Or
> should I send it to somebody for review? Or do I send patches?

CDK classes or packages are more or less maintained... but there is no clear 
maintainer. So, the best thing you can do if you modify code which you did 
not create yourself (so hack away in the HINReader), is to do what you just 
did... contact the maintainer via the cdk-devel@ list, and discuss what you 
want to change...

Especially, you need to discuss changes, if API changes occur, and if the use 
of a class changes... new features, or a smarter implementaton is normally 
fine... (except for algorithms taken from literature, which should not change 
it's implementation ofcourse, but then a smarter implementation can be 
inserted as an second method or second class...)

If unsure, you can send the patch to the mailing list to discuss the changes 
in more detail. But I think you can commit the changes to CVS... 

The proposed changed sounds like a more flexible reading of files; the file 
convertor also tries ChemFile first, I think... and while no RFC is about 
this, IO should provide reading of ChemFile...

The second modification is a new feature, and is a good one. Please add the 
changes to CVS...

Egon