Menu
▾
▴
[Cdk-devel] [ cdk-Patches-3571250 ] CMLReader Patches
|
From: SourceForge.net <no...@so...> - 2012-09-26 16:59:35
|
Patches item #3571250, was opened at 2012-09-24 09:24 Message generated for change (Comment added) made by jwmay You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3571250&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: cdk-1.4.x Group: Accepted Status: Open Resolution: None Priority: 8 Private: No Submitted By: John May (jwmay) Assigned to: Nobody/Anonymous (nobody) Summary: CMLReader Patches Initial Comment: Patch fixes two bugs on the CMLReader (actually it's the CMLCoreModule convention). I have implement two tests to demonstrate the bugs and then provided their respective fixes. I'd consider both bugs pretty major (hence priority 8 :D) as you can't really use CML is the given 'flavour' unless you manually modify it. Atomic number defaulting to 1: https://sourceforge.net/tracker/?func=detail&aid=3553328&group_id=20024&atid=120024 Content of bondStereo ignored: https://sourceforge.net/tracker/?func=detail&aid=3557907&group_id=20024&atid=120024 ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-09-26 09:59 Message: Okay have fixed the implementation with the import and the atomic numbers are now correctly read. File has been reloaded to tracker ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-09-26 09:36 Message: Hmm very strange - I def tested and all was working but tested again now and it's still broken. I can also explain what happened... I had a previous patch that I rebased and missed the import off. As the test fail I realise my fix didn't actually work and the problems go deeper then I thought. It is possible to fix but not well. My fix was that when there is a symbol and no atomic numbers the atomic numbers is set from the symbol. However looking now I see the atomic numbers are always set..... So when the "atom" element has finished reading it does the following (to all internal type arrays). ... if (atomCounter > atomicNumbers.size()) { atomicNumbers.add(null); } ... then when the data is stored/converted the heuristic to check is: if (atomicNumbers.size() == atomCounter) { hasAtomicNumbers = true; } so for "most" data types the size will always equal the atom count as it the number of elements was increased to meet the atom count... which is probably why there is null checks when the array is actually read if (hasAtomicNumbers) { if (atomicNumbers.get(i) != null) currentAtom.setAtomicNumber(Integer.parseInt(atomicNumbers.get(i))); } anyways i've already fixed it so will re-upload. sorry for the issues ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-26 08:52 Message: I can tell what went wrong on my side: I never ran the test. I'm not used to CDK development and CDK testing in particular. I know, John explained to me how to test, but I didn't. That's my half of the issue. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-09-26 08:17 Message: Mmm... OK, one unit test keeps failing, and when I apply the patch, there is a missing import :( Sorry, I cannot look further into it just now... but applying it to cdk-1.4.x does currently not work... not sure what went wrong... ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-09-26 02:27 Message: OK, cool thanx! ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-26 02:19 Message: argl, appended. That's an advantage of pull requests... ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-09-26 02:01 Message: John, didn't you make a unit test too? Ralf, when I look at pull29, I only see the fixes, but no unit tests... ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-26 01:57 Message: OK, here it is: https://github.com/cdk/cdk/pull/29 ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-09-26 01:37 Message: Ralf, pull requests are fine. We need to consolidate in a single place where we let each other know what there is to pull in, though.... I now seem to have at least four places where patches can occur... that is too much, but if you link a pull request from here, that is perfect... I like this tracker to be the consolidated tracker for seeing what is there to review/update/apply... then it does not matter where the patches physically are, as long as they are linked from here. And thanx for the reviewing! ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-26 01:33 Message: Well, it's all fine locally on my machine now, thanks. It's not clear however how I push this to cdk/cdk without making a pull request via rwst/cdk, as Egon requested to me. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-09-25 13:33 Message: [wiki tutorial] yes, would be great. I can also recommend the "git am -3" option, which is for three-way merging... that tends to help me too. ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-09-25 13:14 Message: Not sure if there is a page on the wiki for this - should probably do a tutorial on it... You need to sign it with git. Then when you apply the patches to check the tests you can sign it off as follows. git am patch-file.patch if the patch doesn't apply (normally whitespace) you need to abort and try again ignore white space git am --abort git am --ignore-whitespace patch-file.patch then once everything is checked you can sign off the previous commit using git commit --amend --signoff Egon can probably explain it better but heres the way I understand it: The signoff shows you've applied and checked the patches don't break any tests and perform as expected. If you have a look at the egonw/cdk/master git branch you'll see each commit is signed off by someone else (current master was a commit by me signed by Egon). This serves as a peer review on code - it's seems a bit of pain at first but actually once you do it once it's very quick to do the merging and the time is dependant on the patch size. When a reviewer catches something you missed it pays off and actually the point you see it's benefit. I believe the signing off model comes from the Linux Kernal (Linus doesn't trust many) but the theory is that if you really want to change something in the main branch it needs to be confirmed it doesn't break. If you have a lot of changes to make quickly and can't wait for them to be reviewed (i.e. JChemPaint fork) you can branch off and carry on working (making sure you keep in sync with the upstream repo) and merge back later. ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-25 11:15 Message: You mean hit the Accepted button of this issue? I haven't the rights! ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-09-25 10:59 Message: Could you sign off the commits please, Thanks ---------------------------------------------------------------------- Comment By: Ralf Stephan (rwst) Date: 2012-09-25 10:34 Message: Looks good. Maybe it even resolves some known JCP issue. Please go ahead. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3571250&group_id=20024 |