From: SourceForge.net <no...@so...> - 2009-09-12 16:17:34
|
Patches item #2855925, was opened at 2009-09-10 06:54 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2855925&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 Review Status: Open Resolution: None Priority: 5 Private: No Submitted By: Egon Willighagen (egonw) Assigned to: Rajarshi Guha (rajarshi) Summary: Patch to solve bug #2855850: setStereo(IBond.Stereo) Initial Comment: Aimed as CDK master, so 1 senior review required, all other welcome. The patch changes the CDK API for IBond.set/getStereo() to use IBond.Stereo (new) instead of int. Note that: * there are minor code interpretations in SmilesGenerator, ModelBuilder3D and BondTools * added two unit tests for MDL molfile reading to test the stereo type mapping ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-12 12:17 Message: > but except the API change, there is > no new functionality introduced, except for the two unit tests... do I > understand correctly you like me to split those out into a separate patch? Yes, that's what I originally meant - but it's not a big deal as the core API change is simple. It was late at night, hence my note :) On a related note, I did see one thing this morning. At some points in the patch, the original code had if (Math.abs(bond.getStereo()) < 2) This was checking for 4 constants - STEREO_DOWN, STEREO_UP, STEREO_DOWN_INV and STEREO_UP_INV I'm not sure what the *_INV constants indicate compared to the others. But the new code that you add simply checks whether the value is IBond.Stereo.UP or IBond.Stereo.DOWN - do these enums encompass the *_INV constants in the original code? If not, then the check is missing 2 conditions ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-12 12:11 Message: Rajarshi, I am not entirely what you want me to do. Further splitting up the patch is not really possible, as all code needs to be updated for the API changes... all code should be compilable after each commit. I do understand the patch is very long now, but except the API change, there is no new functionality introduced, except for the two unit tests... do I understand correctly you like me to split those out into a separate patch? Or would you like me to do something else? ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-12 00:25 Message: Looks OK (though it'd be easier if the core changes were in a separate patch). ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2855925&group_id=20024 |