Re: [Geany-Devel] Zombified pull requests
On 2016-01-05 12:46 PM, Jiří Techet wrote: Hi, happy new year and let's celebrate it with something cheerful - zombies! Wouldn't they only be zombies if we closed them and they re-opened themselves? :) I've noticed there are more and more pull requests on github which don't get merged to Geany. It's clear that people are fighting with time to make reviews of pull requests (and Colomban does a great job here!), however, it seems to me there are quite many patches where all the hard work has already been done (review, patch updates) and the only missing thing is the merge - which doesn't happen. I think this is quite unfortunate - there are many patches which might be useful for users; at the same time it might be discouraging for contributors to see their patches unmerged. I've been thinking about what may be the cause of this and several things come to my mind: 1. Not enough feedback from other developers. Typically I am for instance in the mode "I don't want to add too much noise" so unless I love or hate something, I just don't write anything. I think Colomban's own patches suffer from this most as he reviews other people's patches but there's no feedback for his own patches. Maybe it would be a good idea if regular Geany contributors go through the pull requests from time to time and just LGTM those that sound reasonable functionality-wise. I'm not talking about full code review here, just a very quick assessment whether the given feature makes sense for Geany (we should distinguish somehow "I made a review of the patch and LGTM" and "the functionality LTGM to be in Geany"). Agree, I sometimes avoid putting LGTM when I think something is a good idea, because I don't want to give the impression that I have (or even will) reviewed or tested it. Maybe just a "thumbs up" could mean "good idea, though I haven't reviewed or tested it"? To give an example of such a "functionality review", the fractional font sizes patch https://github.com/geany/geany/pull/407 LGTM - even though I probably won't use it myself, I understand it may be useful for someone and it modifies just 23 lines so it's nothing intrusive and can go in from my point of view. Agree, and I've actually wanted for this before. Such a review can be done in a few seconds so if everyone goes through the new pull requests from time to time, the patches will receive some feedback and it will be clearer whether it's something others want it in Geany. 2. Unclear status of some patches. Sometimes it might not be clear in what state the pull request is - I'd suggest adding at least the following two tags: needs-work (reviewed with some comments that need to be addressed) work-in-progress (not meant for review in the current state) This will help to distinguish pull requests awaiting merge and pull requests that aren't there yet. Sounds like a good idea, I just added those labels. We also already have "reviewed", which I believe means whoever added the label has fully reviewed a PR and it's ready to be merged. 3. Fear that the pull request isn't tested enough. I believe that if a patch did undergo a review and there doesn't seem anything obviously wrong with it, it can be merged to master. I think there's no need for some long-term private testing of a patch before it gets merged - people using the development versions of Geany should be aware it may contain bugs and should know how to deal with them. There are also more eyes so a potential bug is spotted earlier. Of course it makes sense getting more conservative towards the end of the development cycle to stabilise things. This is a big one too. Since we don't have a "development" branch (or rather "master" is the development branch), we often have too high standards, only merging stuff that is 100% finished, debugged and ready to release. This kind of defeats the purpose of a development branch. Assuming there's a general consensus that a PR is a good idea, I agree we should start integrating it early, even if there's some possibility it could break things. This way it can get tested in real-life by more than just the person who initiated the PR and others could/would make their own PRs to fixup any perceived issues. When a PR sits off on someone else's repo, it doesn't get real testing, and people (rarely) make PRs against the repo where it came from, where they more likely would if it was in the main repo. OK, these are things that come into my mind regarding the zombie pull requests but there may be other problems. What do you think? Do the points above look reasonable to you and do you think they might help a bit? Is there anything else that might help killing the bloody zombies? 4. Lack of collaboration; We tend not to use Git to its full potential by making pull requests against active pull requests. I don't know if it's a lack of Git knowledge or that it's too much hassle, but we rarely seem to do this (I've only done it a couple tim
Re: [Geany-Devel] Zombified pull requests
Hi Jiri, Its a worthwhile thing to talk about. Some specific comments below, but first a couple of general ones. I occasionally talk to some of the Geany devs/contributors in other forums and its clear that at this moment Geany isn't their primary focus. That is my situation, which is why I concentrate on ML and issue replies, they only take a few minutes at a time, suitable for a break from my other activities, but not a significant time usage. And hopefully that saves other devs and contributors from spending time on initial obvious errors and omissions in reports. Note that if I didn't use the latest Git version of Geany for day to day work I would almost never test it, so testing PRs means including them into the Geany I am using for my other work, which I consider risky if I don't know the contributor or the PR is big, so I don't test them much. Primary contributors or devs PRs only. But I will stop using the latest for day to day work if it becomes unreliable, so I depend on all patches being inspected and tested before commit. And by test I don't mean 5 minutes tick and flick, but *used* for some time, lets say at least a week. So PRs for features/languages I don't use won't get tested either. Colombans #852 is an example at the moment, have incorporated it and if it works fine for a week I'll commit it. And since I am not testing PRs much, I am not in a position to commit them much. But not many people other than committers seem to review/test patches. It should not be just up to the committers to review and test PRs. If there were more people doing that, then there is less pressure on committers to be responsible for the whole thing. A cheery comment "I like this and have been using it for the last month on system XXX with no problems" will go a long way to getting stuff committed. What I do on Asciidoc for PRs that I can't/won't test, is to require at least one person, other than the proposer, to acknowledge usefulness, and to report tested ok in actual use. Then I will commit it, without trying it myself (Colomban spins in his bed :). Of course if a recommender turns out to be unreliable then they will be subsequently ignored, but I haven't had any problems to date. I suspect many of the comments above apply to other devs/contributors as well. On 6 January 2016 at 06:46, Jiří Techet wrote: > Hi, > > happy new year and let's celebrate it with something cheerful - zombies! :) > > I've noticed there are more and more pull requests on github which don't get > merged to Geany. It's clear that people are fighting with time to make > reviews of pull requests (and Colomban does a great job here!), however, it > seems to me there are quite many patches where all the hard work has already > been done (review, patch updates) and the only missing thing is the merge - > which doesn't happen. > > I think this is quite unfortunate - there are many patches which might be > useful for users; at the same time it might be discouraging for contributors > to see their patches unmerged. > > I've been thinking about what may be the cause of this and several things > come to my mind: > > 1. Not enough feedback from other developers. Typically I am for instance in > the mode "I don't want to add too much noise" so unless I love or hate > something, I just don't write anything. I think Colomban's own patches > suffer from this most as he reviews other people's patches but there's no > feedback for his own patches. > > Maybe it would be a good idea if regular Geany contributors go through the > pull requests from time to time and just LGTM those that sound reasonable > functionality-wise. I'm not talking about full code review here, just a very > quick assessment whether the given feature makes sense for Geany (we should > distinguish somehow "I made a review of the patch and LGTM" and "the > functionality LTGM to be in Geany"). I think the categories are: 1) LGTM, I agree with what this PR is doing, how it does it, and I have reviewed and tested it with no issues. This category should mean "can be committed if nobody objects in a reasonable period". 2) LGBI, (looks good by inspection) I agree with what this PR is doing, how it does it, and I have reviewed it, but I havn't tested it. 3) AWTI, (agree with the intent) I agree with the intent, but either havn't looked at the implementation or have issues with it and have provided comments elsewhere. 4) HI, ( despite being cheery, means Hate It :), disagree with its intent and purpose, the implementation is irrelevant. And again I emphasise its not just up to devs/committers to do this. Silence means don't care, somebody else can commit it if they want to and I won't object later (except by providing an alternative PR). > > To give an example of such a "functionality review", the fractional font > sizes patch > > https://github.com/geany/geany/pull/407 > > LGTM - even though I probably won't use it myself, I understand it may be > useful for some
Re: [Geany-Devel] Zombified pull requests
Hi Jiri, Happy new year to you too! It's been much work that needed to be finished before the year's end, and so the pull request has been delayed, that we were talking about. I'll get to it in a few days though. Considering the mentioned zombies there seem to be little hurry at the moment. Cheers, Per ___ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
[Geany-Devel] Zombified pull requests
Hi, happy new year and let's celebrate it with something cheerful - zombies! I've noticed there are more and more pull requests on github which don't get merged to Geany. It's clear that people are fighting with time to make reviews of pull requests (and Colomban does a great job here!), however, it seems to me there are quite many patches where all the hard work has already been done (review, patch updates) and the only missing thing is the merge - which doesn't happen. I think this is quite unfortunate - there are many patches which might be useful for users; at the same time it might be discouraging for contributors to see their patches unmerged. I've been thinking about what may be the cause of this and several things come to my mind: 1. Not enough feedback from other developers. Typically I am for instance in the mode "I don't want to add too much noise" so unless I love or hate something, I just don't write anything. I think Colomban's own patches suffer from this most as he reviews other people's patches but there's no feedback for his own patches. Maybe it would be a good idea if regular Geany contributors go through the pull requests from time to time and just LGTM those that sound reasonable functionality-wise. I'm not talking about full code review here, just a very quick assessment whether the given feature makes sense for Geany (we should distinguish somehow "I made a review of the patch and LGTM" and "the functionality LTGM to be in Geany"). To give an example of such a "functionality review", the fractional font sizes patch https://github.com/geany/geany/pull/407 LGTM - even though I probably won't use it myself, I understand it may be useful for someone and it modifies just 23 lines so it's nothing intrusive and can go in from my point of view. Such a review can be done in a few seconds so if everyone goes through the new pull requests from time to time, the patches will receive some feedback and it will be clearer whether it's something others want it in Geany. 2. Unclear status of some patches. Sometimes it might not be clear in what state the pull request is - I'd suggest adding at least the following two tags: needs-work (reviewed with some comments that need to be addressed) work-in-progress (not meant for review in the current state) This will help to distinguish pull requests awaiting merge and pull requests that aren't there yet. 3. Fear that the pull request isn't tested enough. I believe that if a patch did undergo a review and there doesn't seem anything obviously wrong with it, it can be merged to master. I think there's no need for some long-term private testing of a patch before it gets merged - people using the development versions of Geany should be aware it may contain bugs and should know how to deal with them. There are also more eyes so a potential bug is spotted earlier. Of course it makes sense getting more conservative towards the end of the development cycle to stabilise things. OK, these are things that come into my mind regarding the zombie pull requests but there may be other problems. What do you think? Do the points above look reasonable to you and do you think they might help a bit? Is there anything else that might help killing the bloody zombies? Cheers, Jiri ___ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
