Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inlined test molecule creation from MoleculeFactory.java. #53

Closed
wants to merge 2 commits into from
Closed

Inlined test molecule creation from MoleculeFactory.java. #53

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2014

Inlined test molecule creation prior to adding more tests for a bug, as recommended in a recent discussion with JWM.

@@ -92,14 +139,14 @@ public void testFindHeavyAtomsInChain_IAtomContainer_IAtomContainer() throws Exc

@Test
public void testNumberOfUnplacedHeavyAtoms_IAtomContainer(){
IAtomContainer ac = MoleculeFactory.makeAlphaPinene();
IAtomContainer ac = AtomPlacer3DTest.makeAlphaPinene();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would tend to leave off the static class definition such that this would read IAtomContainer ac = makeAlphaPinene();. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a fine idea,

Mark

From: John May
Sent: Monday, September 01, 2014 6:24 PM
To: cdk/cdk
Cc: mbv31602
Subject: Re: [cdk] Inlined test molecule creation from MoleculeFactory.java. (#53)

In tool/builder3d/src/test/java/org/openscience/cdk/modeling/builder3d/AtomPlacer3DTest.java:

@@ -92,14 +139,14 @@ public void testFindHeavyAtomsInChain_IAtomContainer_IAtomContainer() throws Exc

@test
public void testNumberOfUnplacedHeavyAtoms_IAtomContainer(){

  • IAtomContainer ac = MoleculeFactory.makeAlphaPinene();
    
  • IAtomContainer ac = AtomPlacer3DTest.makeAlphaPinene();
    
    I would tend to leave off the static class definition such that this would read IAtomContainer ac = makeAlphaPinene();. Thoughts?


Reply to this email directly or view it on GitHub.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi John,

Wasn't certain, do you want me to make the changes, or have you started on them yourself?

Mark

From: John May
Sent: Monday, September 01, 2014 6:24 PM
To: cdk/cdk
Cc: mbv31602
Subject: Re: [cdk] Inlined test molecule creation from MoleculeFactory.java. (#53)

In tool/builder3d/src/test/java/org/openscience/cdk/modeling/builder3d/AtomPlacer3DTest.java:

@@ -92,14 +139,14 @@ public void testFindHeavyAtomsInChain_IAtomContainer_IAtomContainer() throws Exc

@test
public void testNumberOfUnplacedHeavyAtoms_IAtomContainer(){

  • IAtomContainer ac = MoleculeFactory.makeAlphaPinene();
    
  • IAtomContainer ac = AtomPlacer3DTest.makeAlphaPinene();
    
    I would tend to leave off the static class definition such that this would read IAtomContainer ac = makeAlphaPinene();. Thoughts?


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the change please. Just add another commit to the same branch and push it. It is then appended to the pull request.

Thanks
J

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changes made and pushed. Tidied up the imports a little.

Mark

From: John May
Sent: Monday, September 01, 2014 7:05 PM
To: cdk/cdk
Cc: mbv31602
Subject: Re: [cdk] Inlined test molecule creation from MoleculeFactory.java. (#53)

In tool/builder3d/src/test/java/org/openscience/cdk/modeling/builder3d/AtomPlacer3DTest.java:

@@ -92,14 +139,14 @@ public void testFindHeavyAtomsInChain_IAtomContainer_IAtomContainer() throws Exc

@test
public void testNumberOfUnplacedHeavyAtoms_IAtomContainer(){

  • IAtomContainer ac = MoleculeFactory.makeAlphaPinene();
    
  • IAtomContainer ac = AtomPlacer3DTest.makeAlphaPinene();
    
    Could you make the change please. Just add another commit to the same branch and push it. It is then appended to the pull request. Thanks J

    Reply to this email directly or view it on GitHub.

@johnmay
Copy link
Member

johnmay commented Sep 1, 2014

Looks good, thanks for the update commit. Applied and pushed.

@johnmay johnmay closed this Sep 1, 2014
@ghost ghost deleted the builder3d/inlining branch September 27, 2014 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant