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