From: SourceForge.net <no...@so...> - 2010-05-16 14:59:08
|
Patches item #2996119, was opened at 2010-05-03 15:49 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2996119&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: master >Group: Accepted >Status: Closed >Resolution: Accepted Priority: 5 Private: No Submitted By: Egon Willighagen (egonw) Assigned to: Nobody/Anonymous (nobody) Summary: Implementation of CLOCKWISE and ANTICLOCKWISE stere Initial Comment: Using the new framework proposed in patch: 2995814 More general stereochemistry framework It introduces a new Java package org.openscience.cdk.stereo with the class ILigancyFourChirality. This is the kind of stereochemistry found in SMILES, which is my next target. Aimed at master. ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2010-05-16 10:59 Message: Patch looks good. Applied and pushed to master ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-05-05 01:36 Message: Updated the patch, applying the suggestions by Gilleain. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-05-04 08:37 Message: Ack on CLOCKWISE, the Stereo enum, and the getLigands() JavaDoc. Will upload a new patch version shortly. Then array copy is to prevent people from changing the data model unintentionally. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2010-05-04 08:32 Message: A very minor thing, but "Clockwise" is a whole word so I would prefer "ANTI_CLOCKWISE" and "CLOCKWISE" rather than "ANTI_CLOCK_WISE" and "CLOCK_WISE". I didn't know the word 'ligancy' but now I do :) Oh, and the convention for Java enums are capitalised lowercase, so 'Stereo' rather than 'STEREO'. I think that the doc comment for getLigands is wrong : "Returns an array IS ligand atoms..." - should it be "array OF ligand atoms"? This is naturally repeated in the interface and implementation doc comments. The only actually code-related point that I can see is the implementation of the getLigands method. Is there a reason for it returning a copy of its atoms, rather than just a reference? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2996119&group_id=20024 |