I left a few comments with the commit. It's a great start, thanks!

Made 2 other small changes, please check!

Also you're probably aware but the unit tests don't all run on Windows, some of them have line ending issues.

Actually no, but I suspected that might the case. Have you got a list?

Looks good, I'm happy with all except the hydrogens. Maybe I'm missing something but isn't the problem here that the input CML didn't specify the count. For example this would give correct behaviour with the existing code:

<atom id="a1" symbol="H" hydrogens="0"/>

If it's documented in the CML spec that hydrogens have 0 implicit hydrogens by default it's okay. Otherwise, this should not be done automatically, there are other methods to set the hydrogen count to allow valid input for SMILES.

Also watch out for formatting (whitespace) changes, these are best in a separate commit otherwise they make it difficult to see what changed.

org.openscience.cdk.cdk-io
org.openscience.cdk.modulesuites.MioTests
org.openscience.cdk.coverage.IoCoverageTest
java.lang.AssertionError: File not found in the classpath: io.javafiles

java.lang.AssertionError: File not found in the classpath: io.javafiles

org.openscience.cdk.io.MDLV2000WriterTest
testSingleDoubletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 2"
but: was "M RAD 1 2 2\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 2"
but: was "M RAD 1 2 2\r"

testMultipleRadicals(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output on line 22
Expected: is "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2"
but: was "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2\r"

java.lang.AssertionError: incorrect radical output on line 22
Expected: is "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2"
but: was "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2\r"

testSingleSingletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

testSingleTripletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

Unit test failures. Looks like the first two might be more serious.

Cheers,

J

On Aug 11, 2014, at 2:39 PM, AndyHowlettGitHub notifications@github.com wrote:

org.openscience.cdk.cdk-io
org.openscience.cdk.modulesuites.MioTests
org.openscience.cdk.coverage.IoCoverageTest
java.lang.AssertionError: File not found in the classpath: io.javafiles

java.lang.AssertionError: File not found in the classpath: io.javafiles

org.openscience.cdk.io.MDLV2000WriterTest
testSingleDoubletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 2"
but: was "M RAD 1 2 2\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 2"
but: was "M RAD 1 2 2\r"

testMultipleRadicals(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output on line 22
Expected: is "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2"
but: was "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2\r"

java.lang.AssertionError: incorrect radical output on line 22
Expected: is "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2"
but: was "M RAD 8 1 2 2 2 3 2 4 2 5 2 6 2 7 2 8 2\r"

testSingleSingletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

testSingleTripletRadical(org.openscience.cdk.io.MDLV2000WriterTest)
java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

java.lang.AssertionError: incorrect radical output
Expected: is "M RAD 1 2 1"
but: was "M RAD 1 2 1\r"

—
Reply to this email directly or view it on GitHub.

Nope the first two are old coverage tests. They're disabled in pom but won't be in the IDE.

Applied and pushed. Thanks.