Re: [Zope-dev] github etiquette
Op 17-09-13 17:15, Marius Gedminas schreef: On Tue, Sep 17, 2013 at 09:35:58AM -0400, Jim Fulton wrote: On Tue, Sep 17, 2013 at 8:58 AM, Marius Gedminas mar...@gedmin.as wrote: FWIW the only reason I'm in favour of self-merges is that this short-circuits the have you signed the ZF committer agreement? dance. Only people who have can merge. Sorry, I don't understand the point you're making. I'll try to explain better. I also feel silly when I ask this question from people with very familiar names. (I feel that I have to do when I don't see ZF membership on their GitHub profile.) So are you saying you don't merge other people's code because you don't want to ask if they're contributors? No. I'm saying the reviewer-merges workflow looks like this: 1. Somebody creates a pull request 2. A reviewer reviews 3. OP fixes 4. Reviewer asks have you signed the agreement? if not please sign 5. OP says he/she signed it 6. Reviewer trusts OP's word and merges (I don't have a good short word for somebody who created the pull request, so I abused Original Poster.) Whereas I'd prefer 1. Somebody creates a pull request 2. A reviewer reviews 3. OP fixes 4. Reviewer says looks good to me, feel free to merge (if you're not a committer already, see http://foundation.zope.org/agreements) 5. OP merges It's not a very strong preference. I can feel myself changing my mind already ;-) OTOH the implicit trust in step 6 makes me a bit uneasy, and I'm not quite sure how to verify the fact of the signing. Wait for the user to show up in https://github.com/zopefoundation?tab=members ? Checking that list seems the best to me. This could be a reason to prefer that the 'pull requester' creates a branch in the original repository within the zopefoundation: if he can do that, it proves he is authorized and has signed the agreement. BTW, I have just now seen that I am on that list in the members tab, but in the greyed out section, which I think means that non-members do not see me listed. There was a button 'Publicize membership' next to my name so I clicked and am now in the top 'full-color' list. :-) Others may want to do the same. -- Maurits van Rees: http://maurits.vanrees.org/ Zest Software: http://zestsoftware.nl ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] github etiquette
Hi, I wonder if there is a formal or informal write-up of how to nicely contribute code to the repositories under the zopefoundation umbrella. Back in the subversion days I would branch a project, amend code, write tests, commit this to the repository, ask for feedback on the list, and, when deemed acceptable, merge my changes back to the project's trunk, perhaps even make a release of the project. Yesterday I wanted to add a small feature to zope.formlib and wondered whether I should: 1) clone the canonical zope.formlib repository, make a branch, do my work, push the changes, ask for feedback on the list and eventually merge the branch to the mainline. or 2) fork the repository, make a branch in the fork, do my work, push the changes to my fork, and issue a pull request. The latter is what I did, without explicitly asking for feedback. Luckily someone did give me feedback (thanks!) :-) Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? My apologies in case I missed an obvious document or reference somewhere that describes the way we should work with the zopefoundation's codebase... kind regards, jw ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 12:04:04PM +0200, Jan-Wijbrand Kolman wrote: I wonder if there is a formal or informal write-up of how to nicely contribute code to the repositories under the zopefoundation umbrella. I don't think so. I'd like to see one. Back in the subversion days I would branch a project, amend code, write tests, commit this to the repository, ask for feedback on the list, and, when deemed acceptable, merge my changes back to the project's trunk, perhaps even make a release of the project. Yesterday I wanted to add a small feature to zope.formlib and wondered whether I should: 1) clone the canonical zope.formlib repository, make a branch, do my work, push the changes, ask for feedback on the list and eventually merge the branch to the mainline. or 2) fork the repository, make a branch in the fork, do my work, push the changes to my fork, and issue a pull request. The latter is what I did, without explicitly asking for feedback. Luckily someone did give me feedback (thanks!) :-) I think a pull request is right. It doesn't matter much if the branch you're asking to pull is in the main repository or in a private fork. Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? I think it's fine to merge own pull requests, provided that somebody +1'd it. (Or if nobody cared for a couple of weeks, even after asking for feedback on the list.) My apologies in case I missed an obvious document or reference somewhere that describes the way we should work with the zopefoundation's codebase... Can we please get the http://foundation.zope.org/ website source on GitHub, so it becomes maintainable by the community? Marius Gedminas -- http://pov.lt/ -- Zope 3/BlueBream consulting and development signature.asc Description: Digital signature ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On 9/17/13 12:04 PM, Jan-Wijbrand Kolman wrote: snip 2) fork the repository, make a branch in the fork, do my work, push the changes to my fork, and issue a pull request. The latter is what I did, without explicitly asking for feedback. Luckily someone did give me feedback (thanks!) :-) Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? This is what I did just now. Kind regards, jw ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On 9/17/13 1:00 PM, Marius Gedminas wrote: On Tue, Sep 17, 2013 at 12:04:04PM +0200, Jan-Wijbrand Kolman wrote: I wonder if there is a formal or informal write-up of how to nicely contribute code to the repositories under the zopefoundation umbrella. I don't think so. I'd like to see one. I think we're writing it Right Now :-) regards, jw ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 7:00 AM, Marius Gedminas mar...@gedmin.as wrote: ... 2) fork the repository, make a branch in the fork, do my work, push the changes to my fork, and issue a pull request. The latter is what I did, without explicitly asking for feedback. Luckily someone did give me feedback (thanks!) :-) I think a pull request is right. Yup. It doesn't matter much if the branch you're asking to pull is in the main repository or in a private fork. Yup. (Apparently, number of forks is a way some people keep score on github, so I'll cynically say a fork is better.) Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? I think it's fine to merge own pull requests, provided that somebody +1'd it. (Or if nobody cared for a couple of weeks, even after asking for feedback on the list.) I strongly prefer that the reviewer do the merge. I'd also really like reviewers to take their responsibility seriously, making comments and suggestions where appropriate. Software review, done well, improves the software, and, more importantly, improves the developers. My apologies in case I missed an obvious document or reference somewhere that describes the way we should work with the zopefoundation's codebase... Can we please get the http://foundation.zope.org/ website source on GitHub, so it becomes maintainable by the community? Oh yes, please! Unfortunately, this will require moving from silva to sphinx. It will also entail separating the public sites from private documents. (The above isn't meant to dis silva, it's just not a good fit for zopefoundation.org.) Jim -- Jim Fulton http://www.linkedin.com/in/jimfulton ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On 9/17/13 1:47 PM, Jim Fulton wrote: I strongly prefer that the reviewer do the merge. I'd also really like reviewers to take their responsibility seriously, making comments and suggestions where appropriate. Software review, done well, improves the software, and, more importantly, improves the developers. I agree - I know I get better from recieving feedback :-) Is this reviewer role something someone takes upon himself? I mean, if I see a pull request for a code base that I know, I could review the request? Or do we acknowledge a group of people that generally do reviews (again formally of informally, I don't mind, I'm not looking for official procedures)? In any case, a second pair of eyes before merging is very helpful! regards, jw p.s. Another thing I noticed: some of the discussion about changes and patches and fixes now shift from the mailinglist to github. This is fine, I guess. ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 07:47:42AM -0400, Jim Fulton wrote: On Tue, Sep 17, 2013 at 7:00 AM, Marius Gedminas mar...@gedmin.as wrote: Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? I think it's fine to merge own pull requests, provided that somebody +1'd it. (Or if nobody cared for a couple of weeks, even after asking for feedback on the list.) I strongly prefer that the reviewer do the merge. FWIW the only reason I'm in favour of self-merges is that this short-circuits the have you signed the ZF committer agreement? dance. Only people who have can merge. I also feel silly when I ask this question from people with very familiar names. (I feel that I have to do when I don't see ZF membership on their GitHub profile.) I'd also really like reviewers to take their responsibility seriously, making comments and suggestions where appropriate. Oh, absolutely. Software review, done well, improves the software, and, more importantly, improves the developers. Marius Gedminas -- http://pov.lt/ -- Zope 3/BlueBream consulting and development signature.asc Description: Digital signature ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 8:32 AM, Jan-Wijbrand Kolman janwijbr...@gmail.com wrote: On 9/17/13 1:47 PM, Jim Fulton wrote: I strongly prefer that the reviewer do the merge. I'd also really like reviewers to take their responsibility seriously, making comments and suggestions where appropriate. Software review, done well, improves the software, and, more importantly, improves the developers. I agree - I know I get better from recieving feedback :-) Is this reviewer role something someone takes upon himself? I mean, if I see a pull request for a code base that I know, I could review the request? These are good questions. I probably don't have satisfying answers. The short answer is that I think people who contribute to a project should view review as one of their duties. For better or worse, this is somewhat informal. If you don't get a review in a timely manner, try posting to the appropriate mailing list to request a review. Or do we acknowledge a group of people that generally do reviews (again formally of informally, I don't mind, I'm not looking for official procedures)? No. p.s. Another thing I noticed: some of the discussion about changes and patches and fixes now shift from the mailinglist to github. This is fine, I guess. Yes, IMO. Jim -- Jim Fulton http://www.linkedin.com/in/jimfulton ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 8:58 AM, Marius Gedminas mar...@gedmin.as wrote: On Tue, Sep 17, 2013 at 07:47:42AM -0400, Jim Fulton wrote: On Tue, Sep 17, 2013 at 7:00 AM, Marius Gedminas mar...@gedmin.as wrote: Now that I mended the pull request, should I merge the pull request myself? Or is the current etiquette that someone else should merge the pull request? I think it's fine to merge own pull requests, provided that somebody +1'd it. (Or if nobody cared for a couple of weeks, even after asking for feedback on the list.) I strongly prefer that the reviewer do the merge. FWIW the only reason I'm in favour of self-merges is that this short-circuits the have you signed the ZF committer agreement? dance. Only people who have can merge. Sorry, I don't understand the point you're making. I also feel silly when I ask this question from people with very familiar names. (I feel that I have to do when I don't see ZF membership on their GitHub profile.) So are you saying you don't merge other people's code because you don't want to ask if they're contributors? I can understand this, but I'd still try to encourage a more review-centric workflow. Also, if a change is trivial, the PR doesn't have to be from a contributor. I understand that triviality isn't always clear. Jim -- Jim Fulton http://www.linkedin.com/in/jimfulton ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On 9/17/13 3:32 PM, Jim Fulton wrote: On Tue, Sep 17, 2013 at 8:32 AM, Jan-Wijbrand Kolman Is this reviewer role something someone takes upon himself? I mean, if I see a pull request for a code base that I know, I could review the request? These are good questions. I probably don't have satisfying answers. The short answer is that I think people who contribute to a project should view review as one of their duties. For better or worse, this is somewhat informal. If you don't get a review in a timely manner, try posting to the appropriate mailing list to request a review. Works for me :-) regards, jw ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] github etiquette
On Tue, Sep 17, 2013 at 09:35:58AM -0400, Jim Fulton wrote: On Tue, Sep 17, 2013 at 8:58 AM, Marius Gedminas mar...@gedmin.as wrote: FWIW the only reason I'm in favour of self-merges is that this short-circuits the have you signed the ZF committer agreement? dance. Only people who have can merge. Sorry, I don't understand the point you're making. I'll try to explain better. I also feel silly when I ask this question from people with very familiar names. (I feel that I have to do when I don't see ZF membership on their GitHub profile.) So are you saying you don't merge other people's code because you don't want to ask if they're contributors? No. I'm saying the reviewer-merges workflow looks like this: 1. Somebody creates a pull request 2. A reviewer reviews 3. OP fixes 4. Reviewer asks have you signed the agreement? if not please sign 5. OP says he/she signed it 6. Reviewer trusts OP's word and merges (I don't have a good short word for somebody who created the pull request, so I abused Original Poster.) Whereas I'd prefer 1. Somebody creates a pull request 2. A reviewer reviews 3. OP fixes 4. Reviewer says looks good to me, feel free to merge (if you're not a committer already, see http://foundation.zope.org/agreements) 5. OP merges It's not a very strong preference. I can feel myself changing my mind already ;-) OTOH the implicit trust in step 6 makes me a bit uneasy, and I'm not quite sure how to verify the fact of the signing. Wait for the user to show up in https://github.com/zopefoundation?tab=members ? I can understand this, but I'd still try to encourage a more review-centric workflow. +1 Also, if a change is trivial, the PR doesn't have to be from a contributor. I understand that triviality isn't always clear. It's completely unclear to me, once the PR goes beyond typo fixes. :( Marius Gedminas -- http://pov.lt/ -- Zope 3/BlueBream consulting and development signature.asc Description: Digital signature ___ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
