From: gilleain t. <gil...@gm...> - 2011-03-18 09:25:25
|
Hi, So, to summarise the proposed changes: 1) Bugfix to pdb_atomtypes.xml : probably this can just be made into a small patch and put on the patch tracker 2) Ordered data structure to store monomers + return type change : again, a small trivial change 3) Proper removal of atom refs from chains and monomers 4) Charge and ChainID bugfixes 5) HETATM added to the structure hierarchy : I think this may require some experimentation to get right 6) STRICT mode parsing in PDBReader, and better warnings/errors : good to get started, but there are a lot of possible parse errors! 7) Renaming of "Strand" class to "Chain" : minor change in some ways, but touches a lot of files. Ok, so the question is : who does these things? :) I am happy to do any, all, or none depending on how confident other people are (Andreas, possible Jules, and even others reading...). Also, anything from patches to git branches would be useful. I guess if anyone just provides patches that they can or want to, and I'll see to the rest. gilleain |
From: Egon W. <ego...@gm...> - 2011-03-18 09:48:43
|
Hi Gilleain, Jules, Andreas, Thanx for taking this forward! I suggest to make patches small, where suitable. See below. Also, where possible, please write unit tests, to ensure things do not get broken later. Moreover, it also helps the discussion as it visualizes the bug/problem/limitation that is solved. On Fri, Mar 18, 2011 at 10:25 AM, gilleain torrance <gil...@gm...> wrote: > 1) Bugfix to pdb_atomtypes.xml : probably this can just be made into a > small patch and put on the patch tracker This would be a good small patch. > 2) Ordered data structure to store monomers + return type change : > again, a small trivial change > 3) Proper removal of atom refs from chains and monomers > 4) Charge and ChainID bugfixes Bug fixes can best be separate patches too, which makes it much easier to see what is fixed and how. If you combine this with API changes, it becomes much harder to see what parts are fixing the bugs, and what is other clean up. > 5) HETATM added to the structure hierarchy : I think this may require > some experimentation to get right > 6) STRICT mode parsing in PDBReader, and better warnings/errors : good > to get started, but there are a lot of possible parse errors! Any start is a good start from my perspective! I'm around to help out here... > 7) Renaming of "Strand" class to "Chain" : minor change in some ways, > but touches a lot of files. This is a very important thing to put into a single patch too! Only the rename in a single patch, not confounded with other clean up. Greatly simplifies reviewing the patch! > Ok, so the question is : who does these things? :) :) I'll keep a distant here, so being free for reviewing of patches. > I am happy to do any, all, or none depending on how confident other > people are (Andreas, possible Jules, and even others reading...). > Also, anything from patches to git branches would be useful. I guess > if anyone just provides patches that they can or want to, and I'll see > to the rest. If people want to learn how to do these things, we can set up a meeting on IRC, walking people through common steps, including signing off patches, allowing you to mark a patch as reviewed and approved. Egon -- Dr E.L. Willighagen Postdoctoral Researcher Institutet för miljömedicin Karolinska Institutet Homepage: http://egonw.github.com/ LinkedIn: http://se.linkedin.com/in/egonw Blog: http://chem-bla-ics.blogspot.com/ PubList: http://www.citeulike.org/user/egonw/tag/papers |
From: Andreas S. <asc...@bi...> - 2011-03-23 18:17:48
|
I'd be happy to provide some/all of the patches. But I'd like to enquire about the correct procedure, especially since I've never used Git before. Cheers, Andreas On 18/03/2011 05:48, Egon Willighagen wrote: > Hi Gilleain, Jules, Andreas, > > Thanx for taking this forward! > > I suggest to make patches small, where suitable. See below. Also, > where possible, please write unit tests, to ensure things do not get > broken later. Moreover, it also helps the discussion as it visualizes > the bug/problem/limitation that is solved. > > On Fri, Mar 18, 2011 at 10:25 AM, gilleain torrance > <gil...@gm...> wrote: >> 1) Bugfix to pdb_atomtypes.xml : probably this can just be made into a >> small patch and put on the patch tracker > > This would be a good small patch. > >> 2) Ordered data structure to store monomers + return type change : >> again, a small trivial change >> 3) Proper removal of atom refs from chains and monomers >> 4) Charge and ChainID bugfixes > > Bug fixes can best be separate patches too, which makes it much easier > to see what is fixed and how. If you combine this with API changes, it > becomes much harder to see what parts are fixing the bugs, and what is > other clean up. > >> 5) HETATM added to the structure hierarchy : I think this may require >> some experimentation to get right >> 6) STRICT mode parsing in PDBReader, and better warnings/errors : good >> to get started, but there are a lot of possible parse errors! > > Any start is a good start from my perspective! I'm around to help out here... > >> 7) Renaming of "Strand" class to "Chain" : minor change in some ways, >> but touches a lot of files. > > This is a very important thing to put into a single patch too! Only > the rename in a single patch, not confounded with other clean up. > Greatly simplifies reviewing the patch! > >> Ok, so the question is : who does these things? :) > > :) > > I'll keep a distant here, so being free for reviewing of patches. > >> I am happy to do any, all, or none depending on how confident other >> people are (Andreas, possible Jules, and even others reading...). >> Also, anything from patches to git branches would be useful. I guess >> if anyone just provides patches that they can or want to, and I'll see >> to the rest. > > If people want to learn how to do these things, we can set up a > meeting on IRC, walking people through common steps, including signing > off patches, allowing you to mark a patch as reviewed and approved. > > Egon > |
From: Egon W. <ego...@gm...> - 2011-03-23 19:39:51
|
Hi Andreas, On Wed, Mar 23, 2011 at 8:14 PM, Andreas Schueller <asc...@bi...> wrote: > I'd be happy to provide some/all of the patches. But I'd like to enquire > about the correct procedure, especially since I've never used Git before. Are you happy with things on a command line? First, make sure git knows who you are: > git config --global user.name "Firstname Lastname" > git config --global user.email "you...@yo..." This is used by 'git commit'. Now, set up a local repository (aka fork, clone, and branch): > git clone https://eg...@gi.../cdk/cdk.git Then make a local branch for the upstream cdk-1.4.x branch: > git checkout -b cdk-1.4.x origin/cdk-1.4.x Now, you can best do all your work in branches, so that your local cdk-1.4.x never has patches that are not in the upstream repository (by default called 'origin'). So, create a branch of the cdk-1.4.x branch: > git checkout -b 1-14x-myVeryFirstPatch cdk-1.4.x The actual branch name doesn't matter... I use a pattern over IncrementedValue-originBranchCodeID-someShortDescriptiveLabel... so: 1-14x-myVeryFirstPatch. The second patch, I would develop in a branch, e.g. called 2-14x-mySecondAndCoolerPatch. The branchCodeID I do to make it clear for myself from with main branch I branched: 14x for cdk-1.4.x and m for master. OK, now it is time to make a hack. Check also this post: http://chem-bla-ics.blogspot.com/2011/03/why-i-like-linux-its-fast-or-quick-typo.html I can recommend to start with a really simple patch... you to get familiar with the commands... so a simply spell check error is what I normally advice... > nano src/main/org/openscience/cdk/Atom.java (or so) Make sure to check what git says you changed: > git status > git diff The first shows the changed files, and the second what in fact changed. The next step is different from CVS and SVN: you need to stage changes as to be commited. Say, you changed Atom.java, you can do: > git add src/main/org/openscience/cdk/Atom.java > git status The second to confirm that Atom.java is now 'staged'. To stage all changed files that are already part of your local git repository, you can also simply do: > git add -u > git status (Do that git status a lot, so that you 'see' what is happening.) Next, once you are happy with the 'staged' changes, you can make the commit: > git commit -m "Fixed a spelling error" Make sure to give a good descriptive message. Nothing like "Fixed bug", but descriptor what bug you fixed, and preferably also how (for spelling errors the above is fine). So something like: "Fixed element symbols for PDB atom types" Here it is clear what was fixed and how. Or: "Renamed Strand to Chain to reflect PDB common naming" describing first how you fixed it, and then what. When that is done, you can check your patch with the following two commands: > git log > git show The last shows the more recent patch. And when you are happy with that too (if not, join #cdk at irc.freenode.net, and I'll talk you through some git wizardy :), then you can convert this patch into a text file you can 'file' (see later): > git format-patch -1 Now, if you have made two patches, you'd do 'git format-patch -2'... if you made many patches, you can also create patches from a certain commit. Note that branches are nothing more than 'aliases' for commit hashes (those long 20 char hexadec codes). But, considering you did the above, and branched from 'cdk-1.4.x', then you can also create patches for your branch with 'git format-patch cdk-1.4.x'. This will create files that look like 0001-SomeCommitMessage.patch. These are ASCII files, and suitable for emailing etc. Now, you file this report. This you can do in various ways: send them as attachments to the cdk-patches@ mailing list. Most people, however, create an entry in the Patch tracker at: http://sourceforge.net/tracker/?group_id=20024&atid=320024 You need a SourceForge account for that (because we were getting too much spam) and be logged in. In both cases, indicate for which branch the patch is aimed (cdk-1.4.x or master, the first in this example). The patch will then be reviewed by others. Those others will basically do something like this: > git am -3 --ignore-whitespace 0001-SomeCommitMessage.patch > git show > ant clean dist-all test-dist-all > git commit --amend --signoff > git format-patch -1 (Assuming all individual steps went fine, and the patch is OK :) And upload the 'signed off' patch back into the tracker, to be applied and committed to the main repository by Rajarshi or me. Is that useful to get you started? Egon -- Dr E.L. Willighagen Postdoctoral Researcher Institutet för miljömedicin Karolinska Institutet Homepage: http://egonw.github.com/ LinkedIn: http://se.linkedin.com/in/egonw Blog: http://chem-bla-ics.blogspot.com/ PubList: http://www.citeulike.org/user/egonw/tag/papers |
From: Andreas S. <asc...@bi...> - 2011-03-24 00:46:50
|
Hi Egon, Great! Many thanks for your efforts. That should get me started. It's good to see that the CDK has come a long way in terms of community communication since I last participated. Good job all! Cheers, Andreas On 23/03/2011 15:39, Egon Willighagen wrote: > Hi Andreas, > > On Wed, Mar 23, 2011 at 8:14 PM, Andreas Schueller > <asc...@bi...> wrote: >> I'd be happy to provide some/all of the patches. But I'd like to enquire >> about the correct procedure, especially since I've never used Git before. > > Are you happy with things on a command line? > > First, make sure git knows who you are: > >> git config --global user.name "Firstname Lastname" >> git config --global user.email "you...@yo..." > > This is used by 'git commit'. > > Now, set up a local repository (aka fork, clone, and branch): > >> git clone https://eg...@gi.../cdk/cdk.git > > Then make a local branch for the upstream cdk-1.4.x branch: > >> git checkout -b cdk-1.4.x origin/cdk-1.4.x > > Now, you can best do all your work in branches, so that your local > cdk-1.4.x never has patches that are not in the upstream repository > (by default called 'origin'). So, create a branch of the cdk-1.4.x > branch: > >> git checkout -b 1-14x-myVeryFirstPatch cdk-1.4.x > > The actual branch name doesn't matter... I use a pattern over > IncrementedValue-originBranchCodeID-someShortDescriptiveLabel... so: > 1-14x-myVeryFirstPatch. The second patch, I would develop in a branch, > e.g. called 2-14x-mySecondAndCoolerPatch. The branchCodeID I do to > make it clear for myself from with main branch I branched: 14x for > cdk-1.4.x and m for master. > > OK, now it is time to make a hack. Check also this post: > > http://chem-bla-ics.blogspot.com/2011/03/why-i-like-linux-its-fast-or-quick-typo.html > > I can recommend to start with a really simple patch... you to get > familiar with the commands... so a simply spell check error is what I > normally advice... > >> nano src/main/org/openscience/cdk/Atom.java > > (or so) > > Make sure to check what git says you changed: > >> git status >> git diff > > The first shows the changed files, and the second what in fact changed. > > The next step is different from CVS and SVN: you need to stage changes > as to be commited. Say, you changed Atom.java, you can do: > >> git add src/main/org/openscience/cdk/Atom.java >> git status > > The second to confirm that Atom.java is now 'staged'. > > To stage all changed files that are already part of your local git > repository, you can also simply do: > >> git add -u >> git status > > (Do that git status a lot, so that you 'see' what is happening.) > > Next, once you are happy with the 'staged' changes, you can make the commit: > >> git commit -m "Fixed a spelling error" > > Make sure to give a good descriptive message. Nothing like "Fixed > bug", but descriptor what bug you fixed, and preferably also how (for > spelling errors the above is fine). So something like: > > "Fixed element symbols for PDB atom types" > > Here it is clear what was fixed and how. Or: > > "Renamed Strand to Chain to reflect PDB common naming" > > describing first how you fixed it, and then what. > > When that is done, you can check your patch with the following two commands: > >> git log >> git show > > The last shows the more recent patch. And when you are happy with that > too (if not, join #cdk at irc.freenode.net, and I'll talk you through > some git wizardy :), then you can convert this patch into a text file > you can 'file' (see later): > >> git format-patch -1 > > Now, if you have made two patches, you'd do 'git format-patch -2'... > if you made many patches, you can also create patches from a certain > commit. Note that branches are nothing more than 'aliases' for commit > hashes (those long 20 char hexadec codes). But, considering you did > the above, and branched from 'cdk-1.4.x', then you can also create > patches for your branch with 'git format-patch cdk-1.4.x'. > > This will create files that look like 0001-SomeCommitMessage.patch. > These are ASCII files, and suitable for emailing etc. > > Now, you file this report. This you can do in various ways: send them > as attachments to the cdk-patches@ mailing list. Most people, however, > create an entry in the Patch tracker at: > > http://sourceforge.net/tracker/?group_id=20024&atid=320024 > > You need a SourceForge account for that (because we were getting too > much spam) and be logged in. > > In both cases, indicate for which branch the patch is aimed (cdk-1.4.x > or master, the first in this example). > > The patch will then be reviewed by others. > > Those others will basically do something like this: > >> git am -3 --ignore-whitespace 0001-SomeCommitMessage.patch >> git show >> ant clean dist-all test-dist-all >> git commit --amend --signoff >> git format-patch -1 > > (Assuming all individual steps went fine, and the patch is OK :) > > And upload the 'signed off' patch back into the tracker, to be applied > and committed to the main repository by Rajarshi or me. > > Is that useful to get you started? > > Egon > |
From: Jules K. <jul...@go...> - 2011-03-24 10:02:31
|
Hi all, Andreas, thanks for the offer for help. I'm looking into the PDBAtomTypes.xml errors today, and plan on making a patch. Egon, thanks for the extended explanation of the patch-making-patch-review process! Just what I was looking for! (Honesty forces me to admit my sign-off of your spelling-fixing was not so thorough that it involved ant-builds. I did check every line of the patchfile to see they were only javadoc spelling changes though... ;-) ) Cheers, Jules On 24 March 2011 02:38, Andreas Schueller <asc...@bi...> wrote: > Hi Egon, > > Great! Many thanks for your efforts. That should get me started. > It's good to see that the CDK has come a long way in terms of community > communication since I last participated. > Good job all! > > Cheers, > Andreas > > On 23/03/2011 15:39, Egon Willighagen wrote: > > Hi Andreas, > > > > On Wed, Mar 23, 2011 at 8:14 PM, Andreas Schueller > > <asc...@bi...> wrote: > >> I'd be happy to provide some/all of the patches. But I'd like to enquire > >> about the correct procedure, especially since I've never used Git > before. > > > > Are you happy with things on a command line? > > > > First, make sure git knows who you are: > > > >> git config --global user.name "Firstname Lastname" > >> git config --global user.email "you...@yo..." > > > > This is used by 'git commit'. > > > > Now, set up a local repository (aka fork, clone, and branch): > > > >> git clone https://eg...@gi.../cdk/cdk.git > > > > Then make a local branch for the upstream cdk-1.4.x branch: > > > >> git checkout -b cdk-1.4.x origin/cdk-1.4.x > > > > Now, you can best do all your work in branches, so that your local > > cdk-1.4.x never has patches that are not in the upstream repository > > (by default called 'origin'). So, create a branch of the cdk-1.4.x > > branch: > > > >> git checkout -b 1-14x-myVeryFirstPatch cdk-1.4.x > > > > The actual branch name doesn't matter... I use a pattern over > > IncrementedValue-originBranchCodeID-someShortDescriptiveLabel... so: > > 1-14x-myVeryFirstPatch. The second patch, I would develop in a branch, > > e.g. called 2-14x-mySecondAndCoolerPatch. The branchCodeID I do to > > make it clear for myself from with main branch I branched: 14x for > > cdk-1.4.x and m for master. > > > > OK, now it is time to make a hack. Check also this post: > > > > > http://chem-bla-ics.blogspot.com/2011/03/why-i-like-linux-its-fast-or-quick-typo.html > > > > I can recommend to start with a really simple patch... you to get > > familiar with the commands... so a simply spell check error is what I > > normally advice... > > > >> nano src/main/org/openscience/cdk/Atom.java > > > > (or so) > > > > Make sure to check what git says you changed: > > > >> git status > >> git diff > > > > The first shows the changed files, and the second what in fact changed. > > > > The next step is different from CVS and SVN: you need to stage changes > > as to be commited. Say, you changed Atom.java, you can do: > > > >> git add src/main/org/openscience/cdk/Atom.java > >> git status > > > > The second to confirm that Atom.java is now 'staged'. > > > > To stage all changed files that are already part of your local git > > repository, you can also simply do: > > > >> git add -u > >> git status > > > > (Do that git status a lot, so that you 'see' what is happening.) > > > > Next, once you are happy with the 'staged' changes, you can make the > commit: > > > >> git commit -m "Fixed a spelling error" > > > > Make sure to give a good descriptive message. Nothing like "Fixed > > bug", but descriptor what bug you fixed, and preferably also how (for > > spelling errors the above is fine). So something like: > > > > "Fixed element symbols for PDB atom types" > > > > Here it is clear what was fixed and how. Or: > > > > "Renamed Strand to Chain to reflect PDB common naming" > > > > describing first how you fixed it, and then what. > > > > When that is done, you can check your patch with the following two > commands: > > > >> git log > >> git show > > > > The last shows the more recent patch. And when you are happy with that > > too (if not, join #cdk at irc.freenode.net, and I'll talk you through > > some git wizardy :), then you can convert this patch into a text file > > you can 'file' (see later): > > > >> git format-patch -1 > > > > Now, if you have made two patches, you'd do 'git format-patch -2'... > > if you made many patches, you can also create patches from a certain > > commit. Note that branches are nothing more than 'aliases' for commit > > hashes (those long 20 char hexadec codes). But, considering you did > > the above, and branched from 'cdk-1.4.x', then you can also create > > patches for your branch with 'git format-patch cdk-1.4.x'. > > > > This will create files that look like 0001-SomeCommitMessage.patch. > > These are ASCII files, and suitable for emailing etc. > > > > Now, you file this report. This you can do in various ways: send them > > as attachments to the cdk-patches@ mailing list. Most people, however, > > create an entry in the Patch tracker at: > > > > http://sourceforge.net/tracker/?group_id=20024&atid=320024 > > > > You need a SourceForge account for that (because we were getting too > > much spam) and be logged in. > > > > In both cases, indicate for which branch the patch is aimed (cdk-1.4.x > > or master, the first in this example). > > > > The patch will then be reviewed by others. > > > > Those others will basically do something like this: > > > >> git am -3 --ignore-whitespace 0001-SomeCommitMessage.patch > >> git show > >> ant clean dist-all test-dist-all > >> git commit --amend --signoff > >> git format-patch -1 > > > > (Assuming all individual steps went fine, and the patch is OK :) > > > > And upload the 'signed off' patch back into the tracker, to be applied > > and committed to the main repository by Rajarshi or me. > > > > Is that useful to get you started? > > > > Egon > > > > > ------------------------------------------------------------------------------ > Enable your software for Intel(R) Active Management Technology to meet the > growing manageability and security demands of your customers. Businesses > are taking advantage of Intel(R) vPro (TM) technology - will your software > be a part of the solution? Download the Intel(R) Manageability Checker > today! http://p.sf.net/sfu/intel-dev2devmar > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel > |
From: Egon W. <ego...@gm...> - 2011-03-24 10:07:29
|
On Thu, Mar 24, 2011 at 11:02 AM, Jules Kerssemakers <jul...@go...> wrote: > Egon, thanks for the extended explanation of the patch-making-patch-review > process! Just what I was looking for! Yeah, I think I'll blog it too... it's basically all over the web, but some targeted version doesn't hurt :) > (Honesty forces me to admit my sign-off of your spelling-fixing was not so > thorough that it involved ant-builds. I did check every line of the > patchfile to see they were only javadoc spelling changes though... ;-) ) No worries! These were just JavaDoc changes indeed, which you checked (great!), so no recompile was really needed here, and Rajarshi and I would normally do that just before committing anyway, and then there is always Nightly to start blaming when it does go wrong. (Back in the SVN days, the repository was frequently broken, this is now rare :) Egon -- Dr E.L. Willighagen Postdoctoral Researcher Institutet för miljömedicin Karolinska Institutet Homepage: http://egonw.github.com/ LinkedIn: http://se.linkedin.com/in/egonw Blog: http://chem-bla-ics.blogspot.com/ PubList: http://www.citeulike.org/user/egonw/tag/papers |