[Cdk-devel] [ cdk-Patches-2881744 ] Signatures package and module

[SourceForge.net <no...@so...>]

Patches item #2881744, was opened at 2009-10-19 09:42
Message generated for change (Comment added) made by rajarshi
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: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: gilleain maclean torrance (gilleain)
Assigned to: Nobody/Anonymous (nobody)
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: Rajarshi Guha (rajarshi)
Date: 2009-10-21 20: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 11: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 11: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-19 21: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