The code is fine, I think. I don't understand the package structure. The package is nu.xom, but it is in cdk source? The patch seems to be from Bioclipse repository (at least cdk-externals sounds like Bioclipse for me), so I can't really see, where this is supposed to end up.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
patch looks OK to me (I was also confused as to why we have to havea nu.xom package in the CDK hierarchy. Egons explanation makes sense). But I cant apply the patch via git am or git apply
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Two reviews please, as I like to see it in CDK 1.2.x.
It's a fairly small patch, and that should not be the problem.
The CustomSerializer must be in org.openscience.cdk.libio.cml (at least not nu.xom), because the xom-1.1.jar is sealed which dissallows that.
The code is fine, I think. I don't understand the package structure. The package is nu.xom, but it is in cdk source? The patch seems to be from Bioclipse repository (at least cdk-externals sounds like Bioclipse for me), so I can't really see, where this is supposed to end up.
There is a nu.xom package because the customized class used for writing the CML is expected to be in that package.
The patch seems from Bioclipse SVN, as it is already used by Bioclipse and originally developed there.
Not sure what you mean with "where this is supposed to end up". Please explain.
patch looks OK to me (I was also confused as to why we have to havea nu.xom package in the CDK hierarchy. Egons explanation makes sense). But I cant apply the patch via git am or git apply
Rajarshi, I will rework my patch to be based on the latest git cdk-1.2.x.
Patch against CDK git cdk-1.2.x as of today.
New patch version uploaded, made against current git cdk-1.2.x.
To be applied with 'git am'.
Looks good. Applied to cdk-1.2.x. Closing this report