#548 Additional unit test for patch 3554480

Accepted
closed
John May
cdk-1.4.x
5
2012-10-30
2012-10-10
John May
No
0 up votes | 0 down votes | 0%
13 comments

This patch provides documentation an additional unit test for Stephan's N Planar 3 atom type patch (#3554480) to ensure explicit hydrogens do not affect the perception.

1 Attachments

Discussion

1 2 > >> (Page 1 of 2)
  • John May
    John May
    2012-10-16

    • milestone: Accepted --> Needs_Review
     
  • John May
    John May
    2012-10-28

    • Milestone: Accepted --> Needs_Review
     
    • labels: master --> master, atom-typing
    • assigned_to: John May
    • branch: --> master
    • milestone: Needs_Review --> Needs_Revision
    • priority: 1 --> 5
     
  • Looks good. One small type; please fix then. Then I'll check for compiling and regressions:

    • hetroatoms -> heteroatoms

    Oh, actually, did I ever tell you about @cdk.inchi ? You can add that JavaDoc to test methods to indicate what molecules is being tested... best to use an InChI with the FixedH layer...

     
  • John May
    John May
    2012-10-29

    Okay have updated the patch. I decided to add comments in the actual method body and restructure the conditionals to make it clearer what was actually happening. Have also included the @cdk.inchi.

     
  • I welcome an later patch for this:

    Assert.assertEquals(pyrrole.getAtom(0).getHybridization().name(), "PLANAR3");

    I did not spot that earlier :(

     
    • John May
      John May
      2012-10-30

      Sorry I'm not following?

       
      • Oh. just that assert takes (expected, actual) rather than the above.

         
        • John May
          John May
          2012-10-30

          Best practise is actually to use assertThat :)

           
          • Ah, yes, that made it into JUnit now...

             
1 2 > >> (Page 1 of 2)