Looks good and happy to apply and push. One thing extra that could be useful is to replace the exception with a call to 'handleError' in the super class.

But then it's not really an error in relaxed mode.

Sent from my iPhone

On 28 Sep 2014, at 21:33, mbv31602 notifications@github.com wrote:

Stuff to read the non-standard ACD/Labs atom label property (M ZZC) in molfiles generated by ACD/ChemSketch. I've tested it fairly extensively, but as MDLV2000Reader is a strategically-important class in the CDK, any further testing would be greatly appreciated.
The new code throws a CDKException if the ZZC property is found while reading in STRICT mode. If preferred, I could change this to just print a warning and skip the labels, just let me know. There were plenty of precendents in the code to go the exception route.

You can merge this Pull Request by running

git pull https://github.com/mbv31602/cdk acd/atomlabels
Or view, comment on, or merge it at:

#69

Commit Summary

Add recognition of ACDLabs ChemSketch atom label property.
Add reading of ACDLabs ChemSketch atom label property from V2000 molfiles.
File Changes

M base/core/src/main/java/org/openscience/cdk/CDKConstants.java (7)
A base/testdata/src/test/resources/data/mdl/chemsketch-all-labelled.mol (19)
A base/testdata/src/test/resources/data/mdl/chemsketch-embedded-space.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-leading-trailing-space.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-longest-label.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-one-label.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-printable-ascii.mol (17)
M storage/io/src/main/java/org/openscience/cdk/io/MDLV2000Reader.java (42)
M storage/io/src/test/java/org/openscience/cdk/io/MDLV2000PropertiesBlockTest.java (13)
M storage/io/src/test/java/org/openscience/cdk/io/MDLV2000ReaderTest.java (135)
Patch Links:

https://github.com/cdk/cdk/pull/69.patch
https://github.com/cdk/cdk/pull/69.diff
—
Reply to this email directly or view it on GitHub.

Thanks for the info, I wasn't aware of the method in the superclass. I've gone with the CDKException at this point as there were precedents in the code, but I could rethink this later. Exception/handleError or skip over the non-standard stuff with a warning (which I would prefer) is less of a coding issue, more design philosophy... Something for a discussion at a later date.

From: John May
Sent: Monday, September 29, 2014 6:31 PM
To: cdk/cdk
Cc: mbv31602
Subject: Re: [cdk] Acd/atomlabels (#69)

Looks good and happy to apply and push. One thing extra that could be useful is to replace the exception with a call to 'handleError' in the super class.

But then it's not really an error in relaxed mode.

Sent from my iPhone

On 28 Sep 2014, at 21:33, mbv31602 notifications@github.com wrote:

Stuff to read the non-standard ACD/Labs atom label property (M ZZC) in molfiles generated by ACD/ChemSketch. I've tested it fairly extensively, but as MDLV2000Reader is a strategically-important class in the CDK, any further testing would be greatly appreciated.
The new code throws a CDKException if the ZZC property is found while reading in STRICT mode. If preferred, I could change this to just print a warning and skip the labels, just let me know. There were plenty of precendents in the code to go the exception route.

You can merge this Pull Request by running

git pull https://github.com/mbv31602/cdk acd/atomlabels
Or view, comment on, or merge it at:

#69

Commit Summary

Add recognition of ACDLabs ChemSketch atom label property.
Add reading of ACDLabs ChemSketch atom label property from V2000 molfiles.
File Changes

M base/core/src/main/java/org/openscience/cdk/CDKConstants.java (7)
A base/testdata/src/test/resources/data/mdl/chemsketch-all-labelled.mol (19)
A base/testdata/src/test/resources/data/mdl/chemsketch-embedded-space.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-leading-trailing-space.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-longest-label.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-one-label.mol (15)
A base/testdata/src/test/resources/data/mdl/chemsketch-printable-ascii.mol (17)
M storage/io/src/main/java/org/openscience/cdk/io/MDLV2000Reader.java (42)
M storage/io/src/test/java/org/openscience/cdk/io/MDLV2000PropertiesBlockTest.java (13)
M storage/io/src/test/java/org/openscience/cdk/io/MDLV2000ReaderTest.java (135)
Patch Links:

https://github.com/cdk/cdk/pull/69.patch
https://github.com/cdk/cdk/pull/69.diff
—
Reply to this email directly or view it on GitHub.
—
Reply to this email directly or view it on GitHub.

If you can commit this code for now, I'll look into handleError and patch later if appropriate.

Applied and pushed - one extra thing I fixed was the resource files. These should go in the module where they are used, the testdata module is an artifact from the ant build.