From: SourceForge.net <no...@so...> - 2010-02-01 11:19:52
|
Patches item #2881744, was opened at 2009-10-19 15:42 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2881744&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: Needs Revision >Status: Closed >Resolution: Wont Fix Priority: 5 Private: No Submitted By: gilleain maclean torrance (gilleain) Assigned to: gilleain maclean torrance (gilleain) Summary: Signatures package and module Initial Comment: The classes relating to signatures are in the first of these patches, and tests are in the second two. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2010-02-01 12:19 Message: This patch is undergoing a rewrite: http://github.com/gilleain/signatures making these patches obsolete. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 13:24 Message: Gilleain, I am about to upload a new .tar with 8 patches, rebased on the latest master, with my 4 patches in that list too... Can you please add the test class annotation? The coverage testing is reporting: signature.OrbitElement#compareTo does not have a test method signature.OrbitElement#compareTo does not have a test method signature.OrbitElement#toString does not have a test method signature.OrbitElement did not have a TestClass annotation signature.Signature#toCanonicalSignatureString does not have a test method signature.Signature#forAtom does not have a test method signature.Signature#forAtom does not have a test method signature.Signature#calculateOrbits does not have a test method signature.Signature#calculateOrbitElements does not have a test method signature.Signature#calculateOrbitElements does not have a test method signature.Signature#toMolecule does not have a test method signature.Signature#toMolecule does not have a test method signature.Signature#calculateOrbitElementsUnsorted does not have a test method signature.Signature#isCanonical does not have a test method signature.Signature#toCanonicalSignatureString has a test method annotation 'testIsCanonical' which is not found in test class: org.openscien ce.cdk.signature.TestSignature signature.Vertex#toString does not have a test method signature.Vertex did not have a TestClass annotation signature.DAG#get does not have a test method signature.DAG#iterator does not have a test method signature.DAG#size does not have a test method signature.DAG#getBestSignatureString does not have a test method signature.DAG#getMaxHeight does not have a test method signature.DAG#getMaxLabel does not have a test method signature.DAG#getRoot does not have a test method signature.DAG#getLargestLabel does not have a test method signature.DAG#getSMAX does not have a test method signature.DAG did not have a TestClass annotation signature.Edge#equals does not have a test method signature.Edge#toString does not have a test method signature.Edge did not have a TestClass annotation signature.TargetMolecularSignature#add does not have a test method signature.TargetMolecularSignature#add does not have a test method signature.TargetMolecularSignature#add does not have a test method signature.TargetMolecularSignature#toString does not have a test method signature.TargetMolecularSignature#size does not have a test method signature.TargetMolecularSignature#compatibleTargetBonds does not have a test method signature.TargetMolecularSignature#compatibleTargetBonds does not have a test method signature.TargetMolecularSignature#getCount does not have a test method signature.TargetMolecularSignature#getBondedSignatures does not have a test method signature.TargetMolecularSignature#getHeight does not have a test method signature.TargetMolecularSignature#getTargetAtomicSignature does not have a test method signature.TargetMolecularSignature#getTargetAtomicSignature does not have a test method signature.TargetMolecularSignature#getTargetAtomicSubSignature does not have a test method signature.TargetMolecularSignature did not have a TestClass annotation signature.Orbit#clone does not have a test method signature.Orbit#toString does not have a test method signature.Orbit#contains does not have a test method signature.Orbit#isEmpty does not have a test method signature.Orbit#iterator does not have a test method signature.Orbit#remove does not have a test method signature.Orbit#sort does not have a test method signature.Orbit#addAtom does not have a test method signature.Orbit#getFirstAtom does not have a test method signature.Orbit#getHeight does not have a test method signature.Orbit#getAtomIndices does not have a test method signature.Orbit#hasLabel does not have a test method signature.Orbit#getLabel does not have a test method signature.Orbit did not have a TestClass annotation signature.TargetAtomicSignature#toString does not have a test method signature.TargetAtomicSignature#toCanonicalSignatureString does not have a test method signature.TargetAtomicSignature#toMolecule does not have a test method signature.TargetAtomicSignature#toMolecule does not have a test method signature.TargetAtomicSignature#getSignatureStringsFromRootChildren does not have a test method signature.TargetAtomicSignature#getSignatureString does not have a test method signature.TargetAtomicSignature#getSubSignature does not have a test method signature.TargetAtomicSignature#getHeight does not have a test method signature.TargetAtomicSignature did not have a TestClass annotation ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-10-23 03:05 Message: > Right, so patch 0001-Improved-javadocs-in-the-signature-package.patch > should be appliable over the others to make the javadocs more complete. Looks good > TestSignature doesn't extend CDKTest, but is that a problem? The only > clear benefit I can see is logging, but I've not used logging... Not a problem per se. I just saw that all test classes were subclassing CDKTest and hence the comment. Egon might be better able to say if its needed. In any case, I think the patches are fine ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2009-10-22 21:54 Message: Right, so patch 0001-Improved-javadocs-in-the-signature-package.patch should be appliable over the others to make the javadocs more complete. I also made the Edge, Node, and DAG classes package private (by removing the "public" modifier). TestSignature doesn't extend CDKTest, but is that a problem? The only clear benefit I can see is logging, but I've not used logging... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-10-22 02:11 Message: Aah OK. Yes, after getting rid of build.xml parts, it all applies cleanly. Patch looks good and tests are all OK. A few comments * shouldn't TestSignature extend some abstract test class (CDKTest?) * There is a lack of Javadocs in a number of source files (OrbitElement, Signature, TargetAtomicSignature). * Finally, I'm assuming that the user visible portion of this package would be Signature (and TargetAtomoicSignature?). Would it be possible to make the helper classes (Edge, Node etc) package protected, so that they wouldn't be visible in the public API? Otherwise good to go ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-10-21 17:12 Message: Gilleain, I have not had time to look at the patches yet... but please do not worry about the build.xml stuff... Rajarshi, please remove the build.xml bits from the patch (with a plain text editor), and I will take care of adding the module to the build system instead... ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2009-10-21 17:02 Message: 1) 0001-New-signature-package.patch is the one with the actual signature code in! 2) I can remove the calls to system.out 3) Yes, they are meant to be applied to master, but I don't know why the one you tried failed :( 4) I hate this patches system... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-10-20 03:34 Message: Patches look good. A few comments * I don't see any code implementing the signatures themselves. Is that in a separate patch? * In 0003, if the tests are working OK, I'd remove the calls to System.out.println (even though they are commented out) However, I can't apply 0001 (and so didn't try the others): error: patch failed: build.xml:994 error: build.xml: patch does not apply I'm assuming these patches were meant to be applied to master (since they implement new parts of the API)? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2881744&group_id=20024 |