digitalmars.D - D pull request review process -- strawman formal definition, query
- Steven Schveighoffer (114/114) May 09 2013 The review process for D pull requests is basically non-existent. We ne...
- Andrej Mitrovic (8/9) May 09 2013 I'm wondering whether we could use bugzilla for this. It has an
- Daniel Murphy (31/50) May 09 2013 I suggest making the process as easy as possible for reviewers. If it i...
- Steven Schveighoffer (44/93) May 09 2013 I agree, and some of the stages I laid out can be notational, not offici...
- Idan Arye (15/36) May 09 2013 According to the description of the Bugzilla service hook on
- Daniel Murphy (7/13) May 10 2013 Quite often bug reports need to be reviewed before being closed. (At lea...
- Daniel Murphy (30/102) May 10 2013 Sounds good.
- Steven Schveighoffer (28/73) May 10 2013 We can't be perfect :) This is a volunteer army. If that is what
- Daniel Murphy (11/30) May 10 2013 Ok, I think this might be different for dmd vs phobos.
- Lars T. Kyllingstad (30/33) May 09 2013 I think each Phobos module should have an official maintainer.
- Lars T. Kyllingstad (8/31) May 09 2013 I agree with most of what you say, except maybe this:
- Jacob Carlborg (4/8) May 10 2013 We could use labels on github for this.
- Jacob Carlborg (7/14) May 10 2013 Github can handle this. You can assign people to a pull request and add
- Steven Schveighoffer (3/15) May 10 2013 That sounds fine too. I wasn't aware of that feature.
- Jacob Carlborg (4/5) May 10 2013 There are milestones as well, if that is of any use.
- Jesse Phillips (3/8) May 10 2013 I believe only issues could have labels applied. Which is fine
- Jacob Carlborg (4/6) May 10 2013 But currently issues are disabled.
- Thomas Koch (17/24) May 11 2013 The best tool for this is gerrit:
The review process for D pull requests is basically non-existent. We need to change this. Without any understanding of how the process works, both reviewers and pull requesters are left to their own devices and assumptions to determine what is going on (see recent anecdote on a pull request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com ). We currently have a nice (informal but defined) process for large library changes, and it seems to work well. We should do the same at a smaller scale, for pull requests. I want to define a process for a requester to follow in order to submit a pull request, including pre-review requirements, objective measurements for acceptance, and review workflow/schedule. We also need a way to have people generally responsible for reviews. At the moment, reviews are ad-hoc, with no clear schedule or assignment. This can result in misconceptions and bit-rotting of pull requests. We need to try and present a friendly and inviting process so that people actually WANT to contribute (see previously mentioned thread). I realize this is a large commitment, but if we don't have some sort of responsibility (myself included), we won't do it. There are a lot of unpulled pull requests, and I think this can be a very detrimental/demotivating thing for a community-driven project. To that end, here is a strawman for discussion. This is for druntime and phobos only, I don't know if this will work for dmd or other projects: 1) pull request prerequisites a) pull must pass auto tester before being reviewed. If you submit a pull request and it fails, you need to fix it before a review can be done. Exceptions can be made if the auto tester isn't able to test the code properly (e.g. you have a pull for phobos and a pull for druntime that depend on each other). b) pull request description is sufficient to understand the reasoning behind the request. c) any related bugzilla requests should be identified. If this fixes a bug, the bug MUST be in bugzilla before pull request is reviewed. It is a good idea to make sure this is actually a bug before creating a pull request! Use the forums/newsgroups. d) If pull request is for a large feature (e.g. replacement or addition of a module), the library change voting process should be followed on the newsgroups instead of this process. 2) pull request review process stages a) submitted - pull request awaiting initial verification of pull request prerequisites. b) unassigned - pull request passes prerequisites, and is awaiting assignment to an approved reviewer (process for assignment TBD) c) ready for review - pull request is assigned, but reviewer has not started looking at it. d) official review - pull request in review by reviewer. Next stage can be e - h. e) needs work - pull request needs work to be able to be accepted (only use this stage if original requester is not immediately responsive). Go back to c after fixed. f) rejected - pull request is rejected. Can be re-opened by another official contributor. g) approved - pull request is approved for pull. h) conditionally approved - pull request is approved, but with minor changes (e.g. comment or ddoc changes). i) pulled/closed - pull request is approved by second reviewer (this does not need to be an in-depth review) 3) Objective measurements required for pull. All pulls must pass these requirements, plan accordingly. a) Passes auto-tester on all platforms. Exceptions can be made if the auto-tester fails because of a limitation in the auto-tester, but must pass unit tests on at least one failed platform if that is the case. b) Code is formatted properly (TBD). c) 2 reviewers must approve of the feature. Secondary review does not need to be in-depth d) License must be boost. Any copyrights must be CORRECTLY attributed to authors in the designated comment area (if you used code from somewhere else, you must include the appropriate copyright information, and the license must be able to be re-licensed as boost). Any large port of code should include a courtesy link to the original project unless the original project's author is OK with not including the link. e) Any related bugzilla entries must be referenced BY LINK in the pull request, and the bugzilla entries must reference BY LINK the pull request. This is the requester's responsibility to set up. f) Any public-facing code must be properly documented for DDoc to generate. At this point, the actual docs don't have to be verified as properly building (because it's freaking annoying to build them). If the auto-tester is able to do this at some point, then this would become a requirement. 4) Reviewers' roles and responsibilities. I am not defining how a reviewer gets assigned, not sure how that should work, and it likely depends on the tool we use. a) When reviewer accepts role as primary reviewer, he/she should review the pull request or decline the role as soon as possible. b) if the role is accepted, the reviewer should review the pull request within a reasonable time frame (what is this, a month? a week?), or at least acknowledge how long they think it should take to get to it. Feedback is very important so contributors are not discouraged. c) once the review has started, for complex pull requests (or issues with the reviewer's schedule) that would require more than a few days or so to review, the reviewer should note this. d) If there are extra volunteer participants during the review, the reviewer should moderate the discussion and comment on review criticisms that are not clear-cut. In other words, the reviewer should be the official word on whether a criticism is valid and requires a change. Not sure if there should be some sort of appeals system. e) Primary reviewer is responsible for ensuring any objective measurements are fulfilled that AREN'T automatically handled (we should handle as much as possible automatically). f) Secondary reviewer does not need to do an in-depth review. This should not take more than 5-10 minutes. g) The primary reviewer is responsible for changing pull-request status in whatever tool we choose to track this. h) One of the reviewers (not sure if it should be primary or secondary) should close any bugzilla bugs fixed by the pull request. Can this be automated? ================== OK, so now to implement this kind of thing, we need to have a collaboration tool that allows tracking the review through its workflow. No idea what the best tool or what a good tool would be. We need something kind of like an issue tracker (can github issue tracker do this?). I don't have a lot of experience with doing online project collaboration except with D. I like trello, but it seems to have not caught on here. -Steve
May 09 2013
On 5/9/13, Steven Schveighoffer <schveiguy yahoo.com> wrote:I like trello, but it seems to have not caught on here.I'm wondering whether we could use bugzilla for this. It has an "assigned" field, but this is usually used for bug assignees. Perhaps some newer version of bugzilla offers a "reviewer" field of some sort.. or maybe we can introduce our own fields (e.g. pull status -- pending review, in review, accepted, etc). The benefit of using bugzilla is that we would have everything located at a central place.
May 09 2013
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message news:op.wwt44fvkeav7ka stevens-macbook-pro.local...The review process for D pull requests is basically non-existent. We need to change this. Without any understanding of how the process works, both reviewers and pull requesters are left to their own devices and assumptions to determine what is going on (see recent anecdote on a pull > request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com ). We currently have a nice (informal but defined) process for large library changes, and it seems to work well. We should do the same at a smaller > scale, for pull requests.I suggest making the process as easy as possible for reviewers. If it is too difficult, people will avoid doing it. I speak from experience as both a reviewer and reviewee.h) One of the reviewers (not sure if it should be primary or secondary) should close any bugzilla bugs fixed by the pull request. Can this be automated?This is what we are currently doing (sort of) and I think it would be better to have the requester be responsible for closing the original bug. This often requires trying each original test case along with other measure to ensure the bug was actually fixed. Sometimes pull requests are only partial fixes and while the requester (hopefully) knows this, the reviewer will have to work all this out in addition to looking at the actual changes.OK, so now to implement this kind of thing, we need to have a collaboration tool that allows tracking the review through its workflow. No idea what the best tool or what a good tool would be. We need something kind of like an issue tracker (can github issue tracker do this?). I don't have a lot of experience with doing online project collaboration except with D. I like trello, but it seems to have not caught on here. -SteveIdeally this goes on bugzilla or github issues so we don't end up fragmenting the information even further. My main problem is when I find I have time and energy to review some pull requests, I never know which ones are ready. I usually find myself looking through them from the front or the back, and rarely get very far. A big improvement would be a two-state system: - Ready for review - Not ready for review The submitter sets it to ready when they think it's done, and it goes to Not Ready when any of the following happens: - An official reviewer decides it needs more work - It fails the autotester (ideally except for those intermittent failures) - The submitter decides it needs more work A reviewer with some spare time then has a short list of 'ready to go' pull requests. This would help a lot with dmd, I don't know about the other projects. A useful third state would be 'Waiting for official review' which means the formal review process for phobos modules and Walter/Andrei approval for dmd enhancements. I assume something this simple can be done with github issues...?
May 09 2013
On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy <yebblies nospamgmail.com> wrote:"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message news:op.wwt44fvkeav7ka stevens-macbook-pro.local...I agree, and some of the stages I laid out can be notational, not official statuses. There are two things I want to accomplish here: 1. Make the process defined and transparent so both reviewers and submitters understand what is going on, and what is expected. 2. Make someone own the review. Without ownership, there will be long delays, and that reflects a sense of disapproval or apathy towards the submitter or his idea, even if that isn't the case. I like how the "feature" review on the newsgroup has a review manager and he sets a deadline. That works out REALLY well. The recent thread on the review non-process outlined a story from the perspective of the submitter that looked to me very different than what actually happened from the reviewer side (reading the comments after the fact).The review process for D pull requests is basically non-existent. We need to change this. Without any understanding of how the process works, both reviewers and pull requesters are left to their own devices and assumptions to determine what is going on (see recent anecdote on a pull > request: http://forum.dlang.org/post/kmdp2n$at4$1 digitalmars.com ). We currently have a nice (informal but defined) process for large library changes, and it seems to work well. We should do the same at a smaller > scale, for pull requests.I suggest making the process as easy as possible for reviewers. If it is too difficult, people will avoid doing it. I speak from experience as both a reviewer and reviewee.I think we should push for full automation. I think it can be done with github (and actually, I think it already automatically records the commit in bugzilla). The problem I see with making the submitter do it is, the submitter may not be active at the time, or may not care. The pull request is done, and he has his fix, but we need to make sure the bug list is updated properly.h) One of the reviewers (not sure if it should be primary or secondary) should close any bugzilla bugs fixed by the pull request. Can this be automated?This is what we are currently doing (sort of) and I think it would be better to have the requester be responsible for closing the original bug.This often requires trying each original test case along with other measure to ensure the bug was actually fixed.Oh! I completely forgot! Another requirement for a pull request to be accepted: g) Any fixed bugs MUST be accompanied by a unit test that fails with the current code but passes with the change. If this is included, the automated tests should cover that.Sometimes pull requests are only partial fixes and while the requester (hopefully) knows this, the reviewer will have to work all this out in addition to looking at the actual changes.These would be more complex cases. I think they would be rare (at least from a library side, not sure about dmd). It's important to note that reviewers are volunteers, and if you are not willing to take the time to do a full review, you should give it to someone else. Perhaps the "description" requirement can be detailed to say "if this is only a partial fix, that should be noted."Ideally this goes on bugzilla or github issues so we don't end up fragmenting the information even further.agreed. I'm really leaning toward github issues since it already integrates with github pull requests (right? never used it before), probably involves less work to automate.My main problem is when I find I have time and energy to review some pull requests, I never know which ones are ready. I usually find myself looking through them from the front or the back, and rarely get very far. A big improvement would be a two-state system: - Ready for review - Not ready for reviewI think this is important. A pull request shouldn't be "Ready" until it's also submitted to the tracking system. The issue should reflect the current state. In other words, you should be searching for unassigned issues in the correct states, not pull requests.The submitter sets it to ready when they think it's done, and it goes to Not Ready when any of the following happens: - An official reviewer decides it needs more work - It fails the autotester (ideally except for those intermittent failures) - The submitter decides it needs more workI think we are on the same page, my stages don't have to be "real" statuses, but it's important to formalize workflow and statuses so both sides know what is going on and what is expected. -Steve
May 09 2013
On Friday, 10 May 2013 at 01:25:16 UTC, Steven Schveighoffer wrote:On Thu, 09 May 2013 18:28:18 -0400, Daniel Murphy <yebblies nospamgmail.com> wrote:According to the description of the Bugzilla service hook on GitHub, it can automatically close bugs via pull requests, but only since version 4.0 of Bugzilla - and the one on d.puremagic is only 3.4.1. I have no idea what it takes to upgrade a Bugzilla - but the benefit of automatically closing bugs would probably justify it. Another thing - in order for the Bugzilla service hook to work, the commit message must refer to it via the syntax "Ticket 123", "Bug 123" or "Tracker item 123" - while most commit messages in current the pull requests refer to it via the "Issue 123" syntax. I think the new review process should enforce proper reference to Bugzilla issues. Anyways, why keep using Bugzilla instead of GitHub issues?"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message news:op.wwt44fvkeav7ka stevens-macbook-pro.local...I think we should push for full automation. I think it can be done with github (and actually, I think it already automatically records the commit in bugzilla). The problem I see with making the submitter do it is, the submitter may not be active at the time, or may not care. The pull request is done, and he has his fix, but we need to make sure the bug list is updated properly.h) One of the reviewers (not sure if it should be primary or secondary) should close any bugzilla bugs fixed by the pull request. Can this be automated?This is what we are currently doing (sort of) and I think it would be better to have the requester be responsible for closing the original bug.
May 09 2013
"Idan Arye" <GenericNPC gmail.com> wrote in message news:eltfkesxpdjgjesrljbm forum.dlang.org...According to the description of the Bugzilla service hook on GitHub, it can automatically close bugs via pull requests, but only since version 4.0 of Bugzilla - and the one on d.puremagic is only 3.4.1. I have no idea what it takes to upgrade a Bugzilla - but the benefit of automatically closing bugs would probably justify it.Quite often bug reports need to be reviewed before being closed. (At least for dmd)Anyways, why keep using Bugzilla instead of GitHub issues?Last I checked github issues still can't do some basic things like moving bugs between components, you can find lists online. I'm sure they will improve this in the future...
May 10 2013
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message news:op.wwugwetleav7ka stevens-macbook-pro.local...1. Make the process defined and transparent so both reviewers and submitters understand what is going on, and what is expected.Sounds good.2. Make someone own the review. Without ownership, there will be long delays, and that reflects a sense of disapproval or apathy towards the submitter or his idea, even if that isn't the case. I like how the "feature" review on the newsgroup has a review manager and he sets a deadline. That works out REALLY well.My impression is that sometimes modules sit around for quite a while waiting for a review manager. How are reviewers going to be assigned and how will it be ensured that they don't let if fall through the cracks?The recent thread on the review non-process outlined a story from the perspective of the submitter that looked to me very different than what actually happened from the reviewer side (reading the comments after the fact).Having a written process we can refer people to will hopefully help with that, even if it is just what we currently have.I don't think this is a problem with dmd, as most pull requests come from the same small group of people. (I think Kenji has done over 1/3)I think we should push for full automation. I think it can be done with github (and actually, I think it already automatically records the commit in bugzilla). The problem I see with making the submitter do it is, the submitter may not be active at the time, or may not care. The pull request is done, and he has his fix, but we need to make sure the bug list is updated properly.h) One of the reviewers (not sure if it should be primary or secondary) should close any bugzilla bugs fixed by the pull request. Can this be automated?This is what we are currently doing (sort of) and I think it would be better to have the requester be responsible for closing the original bug.Sure, but this does not ensure that the tests have full coverage of the problem, or even full coverage of the cases presented in the bug report. This probably applies to dmd more than the others where failures can have very complex interacting causes.This often requires trying each original test case along with other measure to ensure the bug was actually fixed.Oh! I completely forgot! Another requirement for a pull request to be accepted: g) Any fixed bugs MUST be accompanied by a unit test that fails with the current code but passes with the change.If this is included, the automated tests should cover that.I would love to have a way to add a test case, and have the autotester ensure both that it fails with the old version, and works with the new one. (or the other way around for fail-compilation cases) One of my pet projects is an automated regression sourcer finder where you can give it a test case and it attempts to build with the last N releases. Anyway, another time.Right, the really common one in dmd is bugs that apply to D1.Sometimes pull requests are only partial fixes and while the requester (hopefully) knows this, the reviewer will have to work all this out in addition to looking at the actual changes.These would be more complex cases. I think they would be rare (at least from a library side, not sure about dmd). It's important to note that reviewers are volunteers, and if you are not willing to take the time to do a full review, you should give it to someone else.Perhaps the "description" requirement can be detailed to say "if this is only a partial fix, that should be noted."Yeah, I think we can use labels to set the states and it has assignment built in.Ideally this goes on bugzilla or github issues so we don't end up fragmenting the information even further.agreed. I'm really leaning toward github issues since it already integrates with github pull requests (right? never used it before), probably involves less work to automate.If we use github issues and the pulls and issues are linked, this should be fine.My main problem is when I find I have time and energy to review some pull requests, I never know which ones are ready. I usually find myself looking through them from the front or the back, and rarely get very far. A big improvement would be a two-state system: - Ready for review - Not ready for reviewI think this is important. A pull request shouldn't be "Ready" until it's also submitted to the tracking system. The issue should reflect the current state. In other words, you should be searching for unassigned issues in the correct states, not pull requests.I would like real statuses because they are searchable. The wiki is almost good enough here, but having them connected to the actual pull requests and automatically changed when they fail the test suite would be much much better. Maybe this applies better to dmd where the majority are bugfixes instead of enhancements, I don't know. I think we're agreeing on most of it.The submitter sets it to ready when they think it's done, and it goes to Not Ready when any of the following happens: - An official reviewer decides it needs more work - It fails the autotester (ideally except for those intermittent failures) - The submitter decides it needs more workI think we are on the same page, my stages don't have to be "real" statuses, but it's important to formalize workflow and statuses so both sides know what is going on and what is expected. -Steve
May 10 2013
On Fri, 10 May 2013 03:05:31 -0400, Daniel Murphy <yebblies nospamgmail.com> wrote:"Steven Schveighoffer" <schveiguy yahoo.com> wrote in messageWe can't be perfect :) This is a volunteer army. If that is what happens, that is what happens. At least we should make the submitter aware of that likelihood. A weekly check of pull requests that are assigned but stale might be a good idea.2. Make someone own the review. Without ownership, there will be long delays, and that reflects a sense of disapproval or apathy towards the submitter or his idea, even if that isn't the case. I like how the "feature" review on the newsgroup has a review manager and he sets a deadline. That works out REALLY well.My impression is that sometimes modules sit around for quite a while waiting for a review manager. How are reviewers going to be assigned and how will it be ensured that they don't let if fall through the cracks?Up to your discretion as a reviewer what you require. It's impossible to formalize this requirement as a documented/objective measure. I would say that 9 times out of 10, the bug report will contain a minimized example that causes the issue to occur, that should just be included as a unit-test.Oh! I completely forgot! Another requirement for a pull request to be accepted: g) Any fixed bugs MUST be accompanied by a unit test that fails with the current code but passes with the change.Sure, but this does not ensure that the tests have full coverage of the problem, or even full coverage of the cases presented in the bug report. This probably applies to dmd more than the others where failures can have very complex interacting causes.I didn't consider that the auto-tester wouldn't test that the test case fails on the existing code. I suppose that would just be up to the reviewer whether you wanted to go through the effort of testing with that test on the current master branch, or eyeball it and assume it does fail. The most important thing is that a unit test is inserted to verify the fix actually works, and that a regression is caught immediately.If this is included, the automated tests should cover that.I would love to have a way to add a test case, and have the autotester ensure both that it fails with the old version, and works with the new one.That is perfect. A couple people have mentioned we can do this without github issues, the pull requests support labels and assignment. Though mutually exclusive labels might be awkward. Perhaps we should redefine the stages with that in mind.agreed. I'm really leaning toward github issues since it already integrates with github pull requests (right? never used it before), probably involves less work to automate.Yeah, I think we can use labels to set the states and it has assignment built in.I meant not ALL the stages have to be real statuses :) We clearly want something searchable to see the pull requests you are assigned and which ones are ready for review. I will take a look at github issues and github's pull request features to see what level of features we need. -SteveI think we are on the same page, my stages don't have to be "real" statuses, but it's important to formalize workflow and statuses so both sides know what is going on and what is expected.I would like real statuses because they are searchable. The wiki is almost good enough here, but having them connected to the actual pull requests and automatically changed when they fail the test suite would be much much better. Maybe this applies better to dmd where the majority are bugfixes instead of enhancements, I don't know. I think we're agreeing on most of it.
May 10 2013
"Steven Schveighoffer" <schveiguy yahoo.com> wrote in message news:op.wwvee6wleav7ka stevens-macbook-pro.local...Ok, I think this might be different for dmd vs phobos.Sure, but this does not ensure that the tests have full coverage of the problem, or even full coverage of the cases presented in the bug report. This probably applies to dmd more than the others where failures can have very complex interacting causes.Up to your discretion as a reviewer what you require. It's impossible to formalize this requirement as a documented/objective measure. I would say that 9 times out of 10, the bug report will contain a minimized example that causes the issue to occur, that should just be included as a unit-test.I didn't consider that the auto-tester wouldn't test that the test case fails on the existing code. I suppose that would just be up to the reviewer whether you wanted to go through the effort of testing with that test on the current master branch, or eyeball it and assume it does fail. The most important thing is that a unit test is inserted to verify the fix actually works, and that a regression is caught immediately.Absolutely agreed, this is fundamental. After all it isn't really fixed unless you can guarantee it _stays_ fixed. Adding tests also has the nice effect of pushing the burden of keeping it working onto the next person to modify the code. :)That is perfect. A couple people have mentioned we can do this without github issues, the pull requests support labels and assignment. Though mutually exclusive labels might be awkward. Perhaps we should redefine the stages with that in mind.They support labels and assignment because they automatically get an issue created for them. I think you have to turn on issue support in order to actually add labels to the pull requests (or at least I can't find the interface).
May 10 2013
On Thursday, 9 May 2013 at 21:10:55 UTC, Steven Schveighoffer wrote:4) Reviewers' roles and responsibilities. I am not defining how a reviewer gets assigned, not sure how that should work, and it likely depends on the tool we use.I think each Phobos module should have an official maintainer. The modules' documentation have "Author" sections, but those typically list everyone that have ever made a significant contribution to a module, and many of those people have long since withdrawn from the D community. I propose that there be additional "Maintainer" sections which, for each module, specify *one* person who has the primary responsibility for that module. If and when that person disappears from the D community, she or he must be replaced by someone else. Whenever someone makes a pull request, it should be assigned to the maintainer for the module which is most affected by the request. That person also has the primary review responsibility for the request, in the manner described by Steve, but may of course reassign it to someone else if necessary or appropriate. For example, I would be happy to be the official maintainer of std.complex, std.path and std.process. I feel a certain ownership towards those modules, and I very much want to review changes made to them. Unfortunately, time currently does not permit me to scan the forums, Github and Bugzilla every day for discussions, pull requests and bug reports pertaining to these modules. If, however, someone would assign them to me, I would be automatically notified via e-mail, and then I would definitely take the time to deal with it. There will of course be requests that have a large impact on several modules, and there should also be someone that takes care of coordinating the reviews of these. Lars
May 09 2013
On Thursday, 9 May 2013 at 21:10:55 UTC, Steven Schveighoffer wrote:[...] I want to define a process for a requester to follow in order to submit a pull request [...]I agree with most of what you say, except maybe this:2) pull request review process stages a) submitted - pull request awaiting initial verification of pull request prerequisites. b) unassigned - pull request passes prerequisites, and is awaiting assignment to an approved reviewer (process for assignment TBD) c) ready for review - pull request is assigned, but reviewer has not started looking at it. d) official review - pull request in review by reviewer. Next stage can be e - h. e) needs work - pull request needs work to be able to be accepted (only use this stage if original requester is not immediately responsive). Go back to c after fixed. f) rejected - pull request is rejected. Can be re-opened by another official contributor. g) approved - pull request is approved for pull. h) conditionally approved - pull request is approved, but with minor changes (e.g. comment or ddoc changes). i) pulled/closed - pull request is approved by second reviewer (this does not need to be an in-depth review)This seems to require yet another tool besides Bugzilla and GitHub, which I think we should avoid. It's bad enough to have information in two places, without scattering it even more. If we can somehow make it work within the current toolset, then I am all for it.
May 09 2013
On 2013-05-10 08:57, Lars T. Kyllingstad wrote:This seems to require yet another tool besides Bugzilla and GitHub, which I think we should avoid. It's bad enough to have information in two places, without scattering it even more. If we can somehow make it work within the current toolset, then I am all for it.We could use labels on github for this. -- /Jacob Carlborg
May 10 2013
On 2013-05-09 23:10, Steven Schveighoffer wrote:OK, so now to implement this kind of thing, we need to have a collaboration tool that allows tracking the review through its workflow. No idea what the best tool or what a good tool would be. We need something kind of like an issue tracker (can github issue tracker do this?). I don't have a lot of experience with doing online project collaboration except with D. I like trello, but it seems to have not caught on here.Github can handle this. You can assign people to a pull request and add labels to indicate different statuses or whatever you want them to mean. At least I think you can add labels to pull requests. I don't currently have any open pull request for any of my projects so I cannot test it. -- /Jacob Carlborg
May 10 2013
On Fri, 10 May 2013 07:00:08 -0400, Jacob Carlborg <doob me.com> wrote:On 2013-05-09 23:10, Steven Schveighoffer wrote:That sounds fine too. I wasn't aware of that feature. -SteveOK, so now to implement this kind of thing, we need to have a collaboration tool that allows tracking the review through its workflow. No idea what the best tool or what a good tool would be. We need something kind of like an issue tracker (can github issue tracker do this?). I don't have a lot of experience with doing online project collaboration except with D. I like trello, but it seems to have not caught on here.Github can handle this. You can assign people to a pull request and add labels to indicate different statuses or whatever you want them to mean. At least I think you can add labels to pull requests. I don't currently have any open pull request for any of my projects so I cannot test it.
May 10 2013
On 2013-05-10 14:41, Steven Schveighoffer wrote:That sounds fine too. I wasn't aware of that feature.There are milestones as well, if that is of any use. -- /Jacob Carlborg
May 10 2013
On Friday, 10 May 2013 at 11:00:08 UTC, Jacob Carlborg wrote:Github can handle this. You can assign people to a pull request and add labels to indicate different statuses or whatever you want them to mean. At least I think you can add labels to pull requests. I don't currently have any open pull request for any of my projects so I cannot test it.I believe only issues could have labels applied. Which is fine for those using issues since the pull will create an issue.
May 10 2013
On 2013-05-10 20:10, Jesse Phillips wrote:I believe only issues could have labels applied. Which is fine for those using issues since the pull will create an issue.But currently issues are disabled. -- /Jacob Carlborg
May 10 2013
Steven Schveighoffer wrote:OK, so now to implement this kind of thing, we need to have a collaboration tool that allows tracking the review through its workflow. No idea what the best tool or what a good tool would be. We need something kind of like an issue tracker (can github issue tracker do this?). I don't have a lot of experience with doing online project collaboration except with D. I like trello, but it seems to have not caught on here.The best tool for this is gerrit: - http://en.wikipedia.org/wiki/Gerrit_%28software%29#Notable_users - Presentation: http://skillsmatter.com/podcast/home/intro-git-gerrit I've downloaded and cut just the gerrit part: http://koch.ro/temp/gerrit_skillsmatter.mp4 - gerrit vs. github pull requests: http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation Gerrit for D: - Gerrit works on top of Git. - Gerrit is designed to automatically run unit and style checks even before a human reviews the code. - Gerrit tracks the whole history of a change request across all versions of this change. One can see diffs between versions of one particular change request. - Gerrit supports topics to further specify what a change is about Regards, Thomas Koch
May 11 2013