[Geany-Devel] Zombified pull requests

[Jiří Techet]

Hi Lex,

On Wed, Jan 6, 2016 at 4:12 AM, Lex Trotman <elextr at 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 at 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:
> >
> > 1. Not enough feedback from other developers. Typically I am for
> instance in
> > the mode "I don't want to add too much noise" so unless I love or hate
> > something, I just don't write anything. I think Colomban's own patches
> > suffer from this most as he reviews other people's patches but there's no
> > feedback for his own patches.
> >
> > Maybe it would be a good idea if regular Geany contributors go through
> the
> > pull requests from time to time and just LGTM those that sound reasonable
> > functionality-wise. I'm not talking about full code review here, just a
> very
> > quick assessment whether the given feature makes sense for Geany (we
> should
> > distinguish somehow "I made a review of the patch and LGTM" and "the
> > functionality LTGM to be in Geany").
>
> I think the categories are:
>
> 1) LGTM, I agree with what this PR is doing, how it does it, and I
> have reviewed and tested it with no issues.  This category should mean
> "can be committed if nobody objects in a reasonable period".
>
> 2) LGBI, (looks good by inspection) I agree with what this PR is
> doing, how it does it, and I have reviewed it, but I havn't tested it.
>
> 3) AWTI, (agree with the intent) I agree with the intent, but either
> havn't looked at the implementation or have issues with it and have
> provided comments elsewhere.
>
> 4) HI, ( despite being cheery, means Hate It :), disagree with its
> intent and purpose, the implementation is irrelevant.
>

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.
> >
> > 2. Unclear status of some patches. Sometimes it might not be clear in
> what
> > state the pull request is - I'd suggest adding at least the following two
> > tags:
> >
> > needs-work (reviewed with some comments that need to be addressed)
> > work-in-progress (not meant for review in the current state)
> >
> > This will help to distinguish pull requests awaiting merge and pull
> requests
> > that aren't there yet.
>
> Sounds good, makes it easier to skip ones that don't need action,
> needs work already exists, work in progress label added.
>
> >
> > 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.
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.geany.org/pipermail/devel/attachments/20160106/728f58e4/attachment-0001.html>