From: SourceForge.net <no...@so...> - 2012-09-26 09:27:26
|
Patches item #3571250, was opened at 2012-09-24 09:24 Message generated for change (Settings changed) made by egonw 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: 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 |