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
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
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 techet@gmail.com 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:
- 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 someone and it modifies just 23 lines so it's nothing intrusive and can go in from my point of view.
That would be a LGBI in my above taxonomy.
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.
- 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 good, makes it easier to skip ones that don't need action, needs work already exists, work in progress label added.
- 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.
This is where we significantly disagree, if the development version isn't usable for "real work" then it isn't going to get used much at all, and "testing" becomes just a tick and flick "well it didn't crash and seemed to do the right thing, once".
And then the development version becomes less stable has more bugs and gets less used and less tested, and thats a downward spiral.
Thats not to say that simple things should not be easier to add, but we get problems caused by the most innocuous looking patches.
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?
Perhaps we could also just close some of the really old ones that have objections on them. People can still comment and they can be resurrected if the proposer objects that they still want it.
Cheers Lex
Cheers,
Jiri
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
Hi Lex,
On Wed, Jan 6, 2016 at 4:12 AM, Lex Trotman elextr@gmail.com wrote:
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.
This is clear and I understand completely people don't have that much time for Geany (myself included). I wasn't actually talking about the time it takes to get a patch reviewed (which can be longer because people don't have time for Geany, but this is fine) but rather the pull requests that seem to be "done" but they are just unmerged.
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.
I'm definitely not suggesting to make master a collection of crappy and crashing code. But IMO if both the patch author and reviewer tried the code and it doesn't seem to cause any problem and if the patch was reviewed, I think it receives the best field testing in master. Everyone uses Geany in a different way and the reviewer won't probably notice a problem that happens only under some special occasion anyway.
Matthew nicely called the current situation as "too high standards" - I feel Colomban set such a high bar with his great code reviews that everyone else is scared to review the code and all the work is left on Colomban.
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 techet@gmail.com 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:
- 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:
- 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".
- 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.
- 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.
- HI, ( despite being cheery, means Hate It :), disagree with its
intent and purpose, the implementation is irrelevant.
I'm afraid I won't remember all those acronyms :-). Maybe just using plain speech is fine.
And again I emphasise its not just up to devs/committers to do this.
Agree.
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 someone and it modifies just 23 lines so it's nothing
intrusive
and can go in from my point of view.
That would be a LGBI in my above taxonomy.
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.
- 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 good, makes it easier to skip ones that don't need action, needs work already exists, work in progress label added.
- 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.
This is where we significantly disagree, if the development version isn't usable for "real work" then it isn't going to get used much at all, and "testing" becomes just a tick and flick "well it didn't crash and seemed to do the right thing, once".
I don't think we significantly disagree - as I said, I don't want master to become unusable either. I just think the bar might be lowered slightly.
And then the development version becomes less stable has more bugs and gets less used and less tested, and thats a downward spiral.
Thats not to say that simple things should not be easier to add, but we get problems caused by the most innocuous looking patches.
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?
Perhaps we could also just close some of the really old ones that have objections on them. People can still comment and they can be resurrected if the proposer objects that they still want it.
Yeah, or have an "outdated" tag or something.
Cheers,
Jiri
[...]
I'm afraid I won't remember all those acronyms :-). Maybe just using plain speech is fine.
Yes, as I said in another post:
Just say it in plain old English :) "I (do/do not) like the idea, I (have/have not) reviewed the implementation and I (have/have not) tested it on (win/lin/both)." Thats not a committer/dev prerogative, anybody can comment.
Any code or symbol is going to be misunderstood, thumbs up and LGTM mean "good to commit" in other places, we shouldn't use them with some limited meaning that other people don't know.
[...]
I don't think we significantly disagree - as I said, I don't want master to become unusable either. I just think the bar might be lowered slightly.
Yes, it is always going to be a judgement call, being willing to quickly revert commits that break stuff, with no negative implications about the committer would help. I can't think of a single instance where we have reverted a commit (I'm sure Git knows).
Big commits that fail in an environment the developer and committer don't have, (eg win/osx) and need testing by others, could be committed to a branch after reverting them so the person who found the breakage can test them easily and fixup commits made.
Cheers Lex
Just to pass on for those who don't read IRC, Colomban expects to add his input to this thread in a few days after he has finished some other (real world) commitments.
Cheers Lex
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:
- 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.
- 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.
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
5. This one applies to many of your PRs Jiří :) Speaking only for myself here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
6. Monster PRs. This goes along with #3. Sometimes PRs are just too big to be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
7. Lack of committers/commits. There's 7 people who have write access to the repo. Of them, Colomban probably does > 95% of the actual merges/commits. IMO, we either need more people able to assign themselves and merge pull requests, or we need to spread the workload out between those who already can. Personally, when I get some time, I go through and cherry pick the very simple/trivial PRs and try and merge them, but it's not very fair to leave all the really hard ones for Colomban (not to put words in his mouth). If this existing committers don't have enough time or interest to keep on top of pull requests, then all we can do (besides status quo) is to have more interested, trusted developers able to merge pull requests.
Cheers, Matthew Brush
Am 06.01.2016 um 04:32 schrieb Matthew Brush:
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"?
Github is a terrible code review platforms. Other platforms do much better: 1) Differentiate between [x] I have reviewed [x] I have tested [x] I like the change (no test or review) 2) Handle updated changesets without losing comments, to the point that you can even browse older revisions
1) is a problem for reviewers and 2) is a problem for everyone
There are a lot of other, much better code review systems, but I guess we're stuck with github (bought into proprietary solution, anyone?)
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.
- 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.
I can't put labels on github. The feature seems to be limited to those with write access to the repository, so it's useless if we want more reviewers.
- 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.
I agree. Currently there is a culture that everything must be perfect prior to merging. This is fine in theory but simply doesn't scale. It requires endless iterations which in turn require a lot of re-testing, and results in PRs sitting for way longer than necessary. Up to the point where the rate of new PRs can't be handled.
I agree that PRs should be merged earlier, with possible fix-up/follow-up commits in a new PR. This way changes acutally get the testing they need.
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?
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
It's awkward to use because it's not visible on the original PR. It's often hard enough to monitor one PR, let alone recursively monitoring PRs in other repos. Also if there are more than 2 people involved at least some of them don't see the PR-ontop-of-PR. Lastly, if you actually merge those PRs you get a weird history with "Merged Pull Request #XX" from different repos (of course XX changes widely).
I think gists linked to from the original PR would be more natural.
- This one applies to many of your PRs Jiří :) Speaking only for
myself here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
We need other means to ensure quality if nobody is up to reviewing them properly. These code areas could make good use unit tests so that merge changes there can be merged with confidence even if they are not deeply reviewed.
- Monster PRs. This goes along with #3. Sometimes PRs are just too
big to be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Yes, monster PRs are difficult to deal with. But on the other hand I get the response that "I don't like to merge this if nothing uses it right now" (when making a groundwork change required by a later one) when attempting to make smaller, incremental changes. Both views are reasonable. Plus, changes that merely lay the groundwork and don't have any impact itself seem to have very low review priority and are merged with large delay, if at all.
I also have learned that breaking up large changes into lots of small commits doesn't help either. It still makes the PR as a whole rather large and lots of commits are not handy to deal with either.
- Lack of committers/commits. There's 7 people who have write access
to the repo. Of them, Colomban probably does > 95% of the actual merges/commits. IMO, we either need more people able to assign themselves and merge pull requests, or we need to spread the workload out between those who already can. Personally, when I get some time, I go through and cherry pick the very simple/trivial PRs and try and merge them, but it's not very fair to leave all the really hard ones for Colomban (not to put words in his mouth). If this existing committers don't have enough time or interest to keep on top of pull requests, then all we can do (besides status quo) is to have more interested, trusted developers able to merge pull requests.
I agree. The more the merrier, if it helps spreading the workload. Jiří is an awesome candidate, especially since he's the official MAC guy.
I'd volunteer as well. However, given how I'm constantly failing to do good PRs initially or even after 2 or 3 revisions (by Colomban's standards), I'm not sure I'm suitable.
Best regards.
On 2016-01-06 02:44 AM, Thomas Martitz wrote:
Am 06.01.2016 um 04:32 schrieb Matthew Brush:
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"?
Github is a terrible code review platforms. Other platforms do much better:
- Differentiate between [x] I have reviewed [x] I have tested [x] I
like the change (no test or review) 2) Handle updated changesets without losing comments, to the point that you can even browse older revisions
- is a problem for reviewers and 2) is a problem for everyone
There are a lot of other, much better code review systems, but I guess we're stuck with github (bought into proprietary solution, anyone?)
Not strictly. Given enough demand, and buy-in, we could switch to most other such tools. We do have adequate hosting for something better, though I doubt there's much desire to switch to a solution that doesn't support Git as the VCS. For the most part I find Github to be basically adequate myself, though it's not without issues.
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.
- 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.
I can't put labels on github. The feature seems to be limited to those with write access to the repository, so it's useless if we want more reviewers.
If not a misconfiguration, I think this is a bummer.
- 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.
I agree. Currently there is a culture that everything must be perfect prior to merging. This is fine in theory but simply doesn't scale. It requires endless iterations which in turn require a lot of re-testing, and results in PRs sitting for way longer than necessary. Up to the point where the rate of new PRs can't be handled.
I agree that PRs should be merged earlier, with possible fix-up/follow-up commits in a new PR. This way changes acutally get the testing they need.
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?
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
It's awkward to use because it's not visible on the original PR. It's often hard enough to monitor one PR, let alone recursively monitoring PRs in other repos. Also if there are more than 2 people involved at least some of them don't see the PR-ontop-of-PR. Lastly, if you actually merge those PRs you get a weird history with "Merged Pull Request #XX" from different repos (of course XX changes widely).
I agree with the visibility issue. I don't think the sub-history is a big deal, especially since we have a tendency/ability to touch-up such commits to make the branch/PR cleaner (for better or worse). Who cares what PRs were made against a sub-sub-sub-PR, as long as the correct authorship is maintained where warranted for individual patches, and those commits get added to the original PR for full visibility to those watching the original PR.
I think gists linked to from the original PR would be more natural.
For smaller stuff, I agree.
- This one applies to many of your PRs Jiří :) Speaking only for
myself here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
We need other means to ensure quality if nobody is up to reviewing them properly. These code areas could make good use unit tests so that merge changes there can be merged with confidence even if they are not deeply reviewed.
Coincidentally, this is the only area of Geany's source code that contains any kind of tests :)
I wouldn't say nobody is up to reviewing though, Jiří and Colomban, among others, seem to have a good grasp of these off-shoot libraries, I was strictly speaking for myself there.
- Monster PRs. This goes along with #3. Sometimes PRs are just too
big to be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Yes, monster PRs are difficult to deal with. But on the other hand I get the response that "I don't like to merge this if nothing uses it right now" (when making a groundwork change required by a later one) when attempting to make smaller, incremental changes. Both views are reasonable. Plus, changes that merely lay the groundwork and don't have any impact itself seem to have very low review priority and are merged with large delay, if at all.
I think this is where some kind of recognized "road-map" would be useful. Whether it be on the wiki, or some other appropriate venue,if it is documented what the goals and general milestones of larger PRs like this are, then it not only makes it more clear to what end the individual PRs exists, but also lets us track the progress on these larger changes.
I also have learned that breaking up large changes into lots of small commits doesn't help either. It still makes the PR as a whole rather large and lots of commits are not handy to deal with either.
- Lack of committers/commits. There's 7 people who have write access
to the repo. Of them, Colomban probably does > 95% of the actual merges/commits. IMO, we either need more people able to assign themselves and merge pull requests, or we need to spread the workload out between those who already can. Personally, when I get some time, I go through and cherry pick the very simple/trivial PRs and try and merge them, but it's not very fair to leave all the really hard ones for Colomban (not to put words in his mouth). If this existing committers don't have enough time or interest to keep on top of pull requests, then all we can do (besides status quo) is to have more interested, trusted developers able to merge pull requests.
I agree. The more the merrier, if it helps spreading the workload. Jiří is an awesome candidate, especially since he's the official MAC guy.
+1
I'd volunteer as well. However, given how I'm constantly failing to do good PRs initially or even after 2 or 3 revisions (by Colomban's standards), I'm not sure I'm suitable.
I have no problem with you being added either. Quite a few of your changes are relatively major and you would make PRs before merging anyway. It's not surprising, given the magnitude, that some of your PRs get a little bogged down, it's natural (see "road map" above).
IMO, the main thing is to trust committers not to make significant changes without first going through a PR/review, and not merging stuff until they've reviewed/tested as much as feasible, even for their own changes, whenever the PR is deemed "ready".
Cheers, Matthew Brush
Am 06.01.2016 um 12:39 schrieb Matthew Brush:
On 2016-01-06 02:44 AM, Thomas Martitz wrote:
Am 06.01.2016 um 04:32 schrieb Matthew Brush:
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"?
Github is a terrible code review platforms. Other platforms do much better:
- Differentiate between [x] I have reviewed [x] I have tested [x] I
like the change (no test or review) 2) Handle updated changesets without losing comments, to the point that you can even browse older revisions
- is a problem for reviewers and 2) is a problem for everyone
There are a lot of other, much better code review systems, but I guess we're stuck with github (bought into proprietary solution, anyone?)
Not strictly. Given enough demand, and buy-in, we could switch to most other such tools. We do have adequate hosting for something better, though I doubt there's much desire to switch to a solution that doesn't support Git as the VCS. For the most part I find Github to be basically adequate myself, though it's not without issues.
Github itself is fine, it's just its code review solution is seriously lacking. I also didn't mean to question git, or the use of it. I'm saying there are lots of better alternatives in the code review space, however I'm not sure if any of them integrates well with github if you still use it for hosting, issue tracking and pull requests (assuming pull requests via github would still be accepted).
Best regards.
On 6 January 2016 at 20:44, Thomas Martitz kugel@rockbox.org wrote:
Am 06.01.2016 um 04:32 schrieb Matthew Brush:
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"?
Github is a terrible code review platforms. Other platforms do much better:
- Differentiate between [x] I have reviewed [x] I have tested [x] I like
the change (no test or review)
Just say it in plain old English :) "I (do/do not) like the idea, I (have/have not) reviewed the implementation and I (have/have not) tested it on (win/lin/both)." Thats not a committer/dev prerogative, anybody can comment.
- Handle updated changesets without losing comments, to the point that you
can even browse older revisions
Yeah that sucks.
- is a problem for reviewers and 2) is a problem for everyone
There are a lot of other, much better code review systems, but I guess we're stuck with github (bought into proprietary solution, anyone?)
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.
- 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.
I can't put labels on github. The feature seems to be limited to those with write access to the repository, so it's useless if we want more reviewers.
Please just add a comment, its actually clearer anyway since we don't have a well defined semantics for the labels :)
- 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.
I agree. Currently there is a culture that everything must be perfect prior to merging. This is fine in theory but simply doesn't scale. It requires endless iterations which in turn require a lot of re-testing, and results in PRs sitting for way longer than necessary. Up to the point where the rate of new PRs can't be handled.
I agree that PRs should be merged earlier, with possible fix-up/follow-up commits in a new PR. This way changes acutally get the testing they need.
The problem with a development branch and with making master more buggy/less stable is that they will be used less, since its dangerous to use them day to day. So the testing will go down even more.
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?
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
If there is not enough effort to keep up with the commits, there is even less effort available to make pull requests on top of other peoples.
It's awkward to use because it's not visible on the original PR. It's often hard enough to monitor one PR, let alone recursively monitoring PRs in other repos. Also if there are more than 2 people involved at least some of them don't see the PR-ontop-of-PR. Lastly, if you actually merge those PRs you get a weird history with "Merged Pull Request #XX" from different repos (of course XX changes widely).
I think gists linked to from the original PR would be more natural.
Don't know a good solution to this one, maybe if you want to actually make changes, just mail the patches to the OP and let them put them up on the PR.
- This one applies to many of your PRs Jiří :) Speaking only for myself
here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
We need other means to ensure quality if nobody is up to reviewing them properly. These code areas could make good use unit tests so that merge changes there can be merged with confidence even if they are not deeply reviewed.
Yes, better tests for the non-GUI areas would make them easier to change. Your offer to write the tests is accepted :)
- Monster PRs. This goes along with #3. Sometimes PRs are just too big to
be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Yes, monster PRs are difficult to deal with. But on the other hand I get the response that "I don't like to merge this if nothing uses it right now" (when making a groundwork change required by a later one) when attempting to make smaller, incremental changes. Both views are reasonable. Plus, changes that merely lay the groundwork and don't have any impact itself seem to have very low review priority and are merged with large delay, if at all.
I also have learned that breaking up large changes into lots of small commits doesn't help either. It still makes the PR as a whole rather large and lots of commits are not handy to deal with either.
You are right that individual commits on a monster PR don't help much unless they can be reviewed, tested and committed individually.
The way massive changes are handled on Julia is for the OP to put up a PR (or just an issue) with a list of the steps required for the whole change. Individual PRs that make preparatory changes then refer to this so there is a context for them and they can be smaller and easier to review/test.
And wherever possible breaking changes are made in such a way that they can operate in parallel with the existing capability so it can be deprecated for a decent period before removal. On that topic I would like to congratulate you on accepting the challenge of making your recent big PRs retain backward compatibility, its not always easy, and can seem like just an annoyance, so well done.
- Lack of committers/commits. There's 7 people who have write access to
the repo. Of them, Colomban probably does > 95% of the actual merges/commits. IMO, we either need more people able to assign themselves and merge pull requests, or we need to spread the workload out between those who already can. Personally, when I get some time, I go through and cherry pick the very simple/trivial PRs and try and merge them, but it's not very fair to leave all the really hard ones for Colomban (not to put words in his mouth). If this existing committers don't have enough time or interest to keep on top of pull requests, then all we can do (besides status quo) is to have more interested, trusted developers able to merge pull requests.
I agree. The more the merrier, if it helps spreading the workload. Jiří is an awesome candidate, especially since he's the official MAC guy.
I'd volunteer as well. However, given how I'm constantly failing to do good PRs initially or even after 2 or 3 revisions (by Colomban's standards), I'm not sure I'm suitable.
The MOST important thing for a commiter is not killer programming skills, its maturity. Can the person be trusted to not commit their own PRs without others agreement? Can the person be patient enough to wait for comments and other people to test, not everybody is available every day. Does the person understand their own limits, will they ask before committing to an area they have never touched before? Having commit rights is not about getting the persons own PRs into Geany, its about getting others changes in.
So if you think you can handle the above, I don't see why both you and Jiri would not be acceptable (assuming Jiri is interested, I don't know if he was asked :).
Cheers Lex
Best regards.
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
Am 06.01.2016 um 12:47 schrieb Lex Trotman:
I can't put labels on github. The feature seems to be limited to those with write access to the repository, so it's useless if we want more reviewers.
Please just add a comment, its actually clearer anyway since we don't have a well defined semantics for the labels :)
The advantage of labels is that you see them on the PR overview. Whereas with comments you have to open every single PR and look for strings that may indicate a review result. While labels can be named arbitrarily, in the end there's going to be a handful common ones which can be looked out for (I disagree that free-form comments are clearer than labels for indicating actual results).
So comments don't scale, whereas labels could be a real help when looking through the list of open PRs for potentially ready PRs.
- 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.
I agree. Currently there is a culture that everything must be perfect prior to merging. This is fine in theory but simply doesn't scale. It requires endless iterations which in turn require a lot of re-testing, and results in PRs sitting for way longer than necessary. Up to the point where the rate of new PRs can't be handled.
I agree that PRs should be merged earlier, with possible fix-up/follow-up commits in a new PR. This way changes acutally get the testing they need.
The problem with a development branch and with making master more buggy/less stable is that they will be used less, since its dangerous to use them day to day. So the testing will go down even more.
You don't use git head even now with supposedly-perfect merges, so you are not lost if it would become a bit less stable :-)
There are lots of other people which are brave enough, and that's more than the 2 people that test each PR (the reviewer and the author). So it's a still a win.
Of course there should be a minimum level of stability that should be kept.
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?
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
If there is not enough effort to keep up with the commits, there is even less effort available to make pull requests on top of other peoples.
It's awkward to use because it's not visible on the original PR. It's often hard enough to monitor one PR, let alone recursively monitoring PRs in other repos. Also if there are more than 2 people involved at least some of them don't see the PR-ontop-of-PR. Lastly, if you actually merge those PRs you get a weird history with "Merged Pull Request #XX" from different repos (of course XX changes widely).
I think gists linked to from the original PR would be more natural.
Don't know a good solution to this one, maybe if you want to actually make changes, just mail the patches to the OP and let them put them up on the PR.
- This one applies to many of your PRs Jiří :) Speaking only for myself
here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
We need other means to ensure quality if nobody is up to reviewing them properly. These code areas could make good use unit tests so that merge changes there can be merged with confidence even if they are not deeply reviewed.
Yes, better tests for the non-GUI areas would make them easier to change. Your offer to write the tests is accepted :)
Tests are written when 1) a change to the code is prepared and the test verifies that change or b) when a bug is fixed and the test verifies the bug is truly fixed. Just writing tests out of the blue (I don't intend to make a change or fix a bug there) don't work.
However we could start demanding tests as part of PRs (where applicable) and perhaps integrate with Travis / other CI.
On the other hand, demanding tests might drive contributors away.
- Monster PRs. This goes along with #3. Sometimes PRs are just too big to
be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Yes, monster PRs are difficult to deal with. But on the other hand I get the response that "I don't like to merge this if nothing uses it right now" (when making a groundwork change required by a later one) when attempting to make smaller, incremental changes. Both views are reasonable. Plus, changes that merely lay the groundwork and don't have any impact itself seem to have very low review priority and are merged with large delay, if at all.
I also have learned that breaking up large changes into lots of small commits doesn't help either. It still makes the PR as a whole rather large and lots of commits are not handy to deal with either.
You are right that individual commits on a monster PR don't help much unless they can be reviewed, tested and committed individually.
The way massive changes are handled on Julia is for the OP to put up a PR (or just an issue) with a list of the steps required for the whole change. Individual PRs that make preparatory changes then refer to this so there is a context for them and they can be smaller and easier to review/test.
That's a good idea. IIRC you can even draw checkboxes / todo lists in github comments?
However there is another catch: I'm hesitent to make PRs on top of other PRs (even in the same repository). Heck, I don't know even it's possible. The catch is that even if I break large changes into multiple PRs, I can still open the PRs only sequientially, so the 2nd PR after the 1st one is complete and merged. This makes it hard to see upcoming changes in the context of of the early PRs. Is there a solution to this catch?
Best regards.
The advantage of labels is that you see them on the PR overview. Whereas with comments you have to open every single PR and look for strings that may indicate a review result. While labels can be named arbitrarily, in the end there's going to be a handful common ones which can be looked out for (I disagree that free-form comments are clearer than labels for indicating actual results).
So comments don't scale, whereas labels could be a real help when looking through the list of open PRs for potentially ready PRs.
This is true, but I think only those with rights on the repo can use them, but comments are available to everyone.
[..]
You don't use git head even now with supposedly-perfect merges, so you are not lost if it would become a bit less stable :-)
Actually I do ... and its GTK3!!!!!! :)
[...]
Yes, better tests for the non-GUI areas would make them easier to change. Your offer to write the tests is accepted :)
Tests are written when 1) a change to the code is prepared and the test verifies that change or b) when a bug is fixed and the test verifies the bug is truly fixed. Just writing tests out of the blue (I don't intend to make a change or fix a bug there) don't work.
However we could start demanding tests as part of PRs (where applicable) and perhaps integrate with Travis / other CI.
On the other hand, demanding tests might drive contributors away.
Yeah, its a bit of a no-win, but we should always ask (if the change is in a non-GUI part).
- Monster PRs. This goes along with #3. Sometimes PRs are just too big
to be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Yes, monster PRs are difficult to deal with. But on the other hand I get the response that "I don't like to merge this if nothing uses it right now" (when making a groundwork change required by a later one) when attempting to make smaller, incremental changes. Both views are reasonable. Plus, changes that merely lay the groundwork and don't have any impact itself seem to have very low review priority and are merged with large delay, if at all.
I also have learned that breaking up large changes into lots of small commits doesn't help either. It still makes the PR as a whole rather large and lots of commits are not handy to deal with either.
You are right that individual commits on a monster PR don't help much unless they can be reviewed, tested and committed individually.
The way massive changes are handled on Julia is for the OP to put up a PR (or just an issue) with a list of the steps required for the whole change. Individual PRs that make preparatory changes then refer to this so there is a context for them and they can be smaller and easier to review/test.
That's a good idea. IIRC you can even draw checkboxes / todo lists in github comments?
Yep, and tick 'em off as they are done.
However there is another catch: I'm hesitent to make PRs on top of other PRs (even in the same repository). Heck, I don't know even it's possible. The catch is that even if I break large changes into multiple PRs, I can still open the PRs only sequientially, so the 2nd PR after the 1st one is complete and merged. This makes it hard to see upcoming changes in the context of of the early PRs. Is there a solution to this catch?
I'm not aware of a technical solution to that, indeed sometimes if the OP is impatient there will be PRs that depend on preceding PRs if they take a while to commit, but at least you can see the dependence in the overview description and sort of expect it.
Best regards. _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 2016-01-06 03:47 AM, Lex Trotman wrote:
On 6 January 2016 at 20:44, Thomas Martitz kugel@rockbox.org wrote:
[...] I agree that PRs should be merged earlier, with possible fix-up/follow-up commits in a new PR. This way changes acutally get the testing they need.
The problem with a development branch and with making master more buggy/less stable is that they will be used less, since its dangerous to use them day to day. So the testing will go down even more.
I personally don't mind the odd bug cropping up here and there on a project which I actively contribute and keep up to date with the bleeding-edge development code. Of course if you're doing mission-critical stuff, you don't want to be using the bleeding-edge, potentially buggy code anyway, the latest release from your distro is much less likely to cause grief.
- Monster PRs [...]
Yes, monster PRs are difficult to deal with [...]
You are right that individual commits on a monster PR don't help much unless they can be reviewed, tested and committed individually.
The way massive changes are handled on Julia is for the OP to put up a PR (or just an issue) with a list of the steps required for the whole change. Individual PRs that make preparatory changes then refer to this so there is a context for them and they can be smaller and easier to review/test.
+1
- Lack of committers/commits [...]
I agree. The more the merrier, if it helps spreading the workload. Jiří is an awesome candidate, especially since he's the official MAC guy.
I'd volunteer as well. However, given how I'm constantly failing to do good PRs initially or even after 2 or 3 revisions (by Colomban's standards), I'm not sure I'm suitable.
The MOST important thing for a commiter is not killer programming skills, its maturity. Can the person be trusted to not commit their own PRs without others agreement? Can the person be patient enough to wait for comments and other people to test, not everybody is available every day. Does the person understand their own limits, will they ask before committing to an area they have never touched before? Having commit rights is not about getting the persons own PRs into Geany, its about getting others changes in.
+1
So if you think you can handle the above, I don't see why both you and Jiri would not be acceptable (assuming Jiri is interested, I don't know if he was asked :).
+1
Cheers, Matthew Brush
Hi Matthew,
On Wed, Jan 6, 2016 at 4:32 AM, Matthew Brush mbrush@codebrainz.ca wrote:
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? :)
Ah, right, I wasn't fully aware of the zombie lifecycle :-).
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:
- 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"?
Yeah, sounds better than LGBI's :-)
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.
- 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.
- 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?
- 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 times, Colomban does it more often I think). Instead we make comments like "you should do this or that", assuming that the only person who must make changes is the pull requester. If instead of making "do this" comments, we made pull requests to show/implement it, it would give something concrete to discuss and increase collaboration on the code.
- This one applies to many of your PRs Jiří :) Speaking only for myself
here, any PRs made against CTags or TagManager I pretty much ignore. Not only that I'm completely unfamiliar with their code, but also the changes are often very subtle and require in-depth knowledge of those code bases, and I'm neither qualified nor inclined to make any judgments about the PR, let alone review or merge them.
Yeah, the TM stuff is an exception - the code is a bit tricky and it definitely needs some more time to get familiar with what TM actually does. I agree that it may be hard to assess just by quick looking at the patches whether it's "something we want" and whether "the patch does the right thing". Fortunately it's mostly reviewed by Colomban now.
In this area my long-term plan is to _completely_ sync the universal-ctags and geany ctags implementations so the ctags maintenance would become just a matter of gettting the latest code from universal-ctags. But there will be many patches on the way to be reviewed...
- Monster PRs. This goes along with #3. Sometimes PRs are just too big to
be digested by volunteer developers all at once. If we started integrating such larger changes earlier, and in smaller pieces, I think it would increase the likelihood of getting the feature/changes merged into the master branch. When a PR is so big, it basically takes as much time to fully review and test it as it did to make the changes in the first place. I personally rarely merge pull requests unless I've reviewed and understand each line of code that changed, and tested all the cases I can think of. As volunteer like the rest of us, I just simply can't afford to spend days/weeks reviewing, understanding, and testing such large PRs, especially if I'm rather indifferent on the feature/improvement they implement.
Again, this is the case of many of my TM patches (I think a similar situation was with the spawn module where the problems were very subtle too). In these cases I think someone just has to be the hero and make the review (and I really appreciate Colomban's work here).
- Lack of committers/commits. There's 7 people who have write access to
the repo. Of them, Colomban probably does > 95% of the actual merges/commits. IMO, we either need more people able to assign themselves and merge pull requests, or we need to spread the workload out between those who already can. Personally, when I get some time, I go through and cherry pick the very simple/trivial PRs and try and merge them, but it's not very fair to leave all the really hard ones for Colomban (not to put words in his mouth). If this existing committers don't have enough time or interest to keep on top of pull requests, then all we can do (besides status quo) is to have more interested, trusted developers able to merge pull requests.
I'm definitely the one to blame here as I have been resisting becoming a "full" developer with a commit access. I was worried I could easily make mess in master by doing "git push origin master" without much thinking but I think I can overcome this by not having the master repo at "origin". So yeah, maybe it's a good idea if I get commit access too. Now I'm afraid the same problems like others are facing apply to me too:
1. Lack of time 2. Lack of knowledge of some stuff (will definitely stay away from anything autotools-related :-) 3. Incapability of producing as great reviews as Colomban :-)
Cheers,
Jiri
On 06.01.2016 04:32, Matthew Brush wrote:
- Lack of collaboration; We tend not to use Git to its full potential
by making pull requests against active pull requests
How to do this? If I find time I doing some improvements on local branch and sending it to PR upstream author via another PR as e.g. done with https://github.com/smostertdev/geany-plugins/pull/1 Is this what you were thinking of or is there another thing?
Cheers, Frank