QEP 52- Commit rule for new feature proposal

[mhugo]

QGIS Enhancement 52 (was 33): Commit rule for new feature proposal

Date 11-06-2015
Author Hugo Mercier
Contact hugo dot mercier at oslandia dot com
Last Edited
Status
Version

Proposed change

This QEP is about a new rule for the commit of code change for a new feature.

The rule is the following: anyone proposing a patch that introduces a new feature, including developers with write access, must send it as a Pull Request to the QGIS github project.

In case the Pull Request is opened by a core developer, it is required (s)he:

• either requests a review (by tagging it)
• or includes a "merge date" information, delay after which (s)he intends to merge the changes

This "quarantine" delay allows other developers to be aware of the proposed changes and react and points problems if any. The delay must represent at least 2 (?) days, and should reflect the time needed to read it. The more important the proposed changes are, the longer the delay.

If accepted, the CODING document will be updated to reflect this new rule.

Voting History

[NathanW2]

Does this apply to all features though? Does something minor need to go though this process?

[mhugo]

Indeed the intent is to avoid the inclusion of important features without notice.
But it does not have to slow down too much the overall commit process. And I don't know how to define what is an "important" feature ...

[NathanW2]

I guess it depends on the impact a feature can have. So something like this: Add other composer templates loading paths isn't really a big deal or that complex. But say adding downloading templates from a online service might be considered more complex and up for review on how it works.

[NathanW2]

However saying that this is s a +1 from me for this process.

[timlinux]

+1 from me. In InaSAFE we use (inspired by @mbernasocchi) a simple process where as PR's are made one of your peers must comment LGTM (looks good to me) before you can merge it. Original submitter always does the merge if they have write permissions so that they can deal with any merge conflicts.

And yes, all commits should be made via PR's. Though I would say for trivial PR's (e.g. a one line change made by @jef-n while preparing the release) the originator could make the PR and then immediately merge it.

[nyalldawson]

And yes, all commits should be made via PR's. Though I would say for trivial PR's (e.g. a one line change made by @jef-n while preparing the release) the originator could make the PR and then immediately merge

What's the reasoning here ( PR and then immediately merge)? Is it so Travis can run?

[timlinux]

@nyalldawson

What's the reasoning here ( PR and then immediately merge)? Is it so Travis can run?

Yes - the reason is that nothing should be ever merged to master without a green dot for travis - and to have the audit trail to prove it...

[mhugo]

@NathanW2 The commit you mention touches the public API (well not that much - only one method in QgsApplication) so could require a PR. But I agree this commit is not that complex.

PR's are made one of your peers must comment LGTM (looks good to me) before you can merge it

@timlinux Does it mean a review for each proposal ? I am not sure there is enough people to review everything. The idea was: by default everything coming from a core dev is accepted, but a small delay allows others to react / disagree on a change.

About PR for all commits in order to trigger Travis: it can be configured to trigger a build for each commit (probably already the case). What would be the difference to require a PR ?

[elpaso]

This is what I think would fit best for (not trivial) [FEATURE] requests.

For core devs:
the core devs [FEATURE] PR should go into a quarantine queue and automatically approved if nobody raise an exception during the quarantine (LGTM peer comment is also an option but I'd still prefer an automatic approval after a fixed time)

For not core devs:
the not-core devs [FEATURE] should be first be submitted as QEP (so that the submitter can know in advance that he/she has a chance that if not poorly coded the PR will be accepted)
after the QEP is approved, then the PR will go into a code review queue managed by QGIS.ORG
the code review will be delegated/outsourced to (in order of availability):

• volunteers in QGIS community (not paid)
• core devs (paid)
• other not-core devs QGIS contributors (paid)

QGIS.ORG will get paid to manage and coordinate the code review queue but QGIS.ORG will not do the code review directly, this is mainly because it's difficult to find a single person that can review code for all the different areas of QGIS code and because delegating this activity to the most skilled core dev in the affected areas would be the best and a reward for core devs activity.

[mbernasocchi]

@mhugo:
About PR for all commits in order to trigger Travis: it can be configured to trigger a build for each commit (probably already the case). What would be the difference to require a PR ?

The main difference would be that master is never broken (at least from the ci point of view)

[nyalldawson]

@mbernasocchi at least.... until homebrew is down something and causes all the osx tests to fail (like now) ;)

[timlinux]

@timlinux Does it mean a review for each proposal ? I am not sure there is enough people to review everything. The idea was: by default everything coming from a core dev is accepted, but a small delay allows others to react / disagree on a change.

@mhugo In our experience with @mbernasocchi 's method in InaSAFE, it is simply a matter of saying 'hey Bob, would you mind giving my PR a once-over?'. Normally someone jumps over to look at it pretty quickly like that. Having PR review is good because we often pick up small mistakes like typos or poorly named methods which are normally missed because the author is 'too close to the code'.

About PR for all commits in order to trigger Travis: it can be configured to trigger a build for each commit (probably already the case). What would be the difference to require a PR ?

Just to know that all tests pass before code is put in master.

[nyalldawson]

The delay must represent at least 2 (?) days, and should reflect the time needed to read it. The more important the proposed changes are, the longer the delay

Can we put a note here that the delay can be extended upon request? Eg, if someone submits a PR which a dev is interested in looking over, but the dev needs more time/are on holidays/etc, they can request a longer delay when required.

[nyalldawson]

least 2 (?) days

also can we make this 2 work days (excluding weekends)

[nyalldawson]

I'm in favor of this in general. My only concern knowing how large a change must be before this rule applies. Is this just a "in-good-faith"/trust the devs type thing to make their own judgement call?

[mhugo]

@timlinux @nyalldawson What about:

• Every commit needs a PR first
• For bug fix, it can be merged when travis is ok
• If the change introduces a new feature, a quarantine delay applies
  • if another core dev says ok to merge, it can be merged before the end of the delay
  • another dev may request a delay extension (preferably quickly after the PR is submitted)
  • if no reaction, it can be merged by the issuer after the delay

[jef-n]

What about going on like we ever did? Are there examples for past problems this would have avoided? Is it worth the hassle?

[3nids]

@jef-n because

• the project got too big and that it's hard to follow everything
• recent log history shows that it does not work anymore: things have been merged without conscensus

that's how I see it

[jef-n]

examples?

[3nids]

node tool switched back to drag'n'drop
new geometry classes just before feature freeze

[jef-n]

Wouldn't the nodetool change have passed through too? I guess people started to complain after there were builds to try - while a QEP would probably have looked fine.

Big feature just before the feature freeze is bad indeed - probably also because the period where there are no builds is also not long enough. I don't see that this QEP would have avoided this either.

Another thing is that 2 days for discussion is not enough, if it's a topic worth discussing (and why is the weekend excluded? maybe it should ready at least 2 days including a weekend).

But as always it's hard (impossible?) to put "common sense" into (short, to the point) rules - and that's a problem I have with most QEPs that try to make sure that everyone behaves right.

[mhugo]

Another thing is that 2 days for discussion is not enough, if it's a topic worth discussing (and why is the weekend excluded? maybe it should ready at least 2 days including a weekend).

I first mentioned a week. But it really depends on the change. 2 days is a proposed minimum. The PR submitter has to set a delay which is in proportion to the "size" of the PR and anyone can request an extension.
And ... I also agree to exclude weekends, I personally try to avoid working during nights and weekends.

[NathanW2]

For me I don't think it's a bad idea for others to eye over what we are all doing. Others might have ideas or feedback on work before it hits master especially if it changes how something works.

[jef-n]

But above there was discussion about excluding small things from this - which obviously makes sense - but of course also makes it unclear to what this QEP would apply. There's already a general suggestion to do QEPs for bigger changes (also unclear where "big" starts).

[jef-n]

@3nids and for the nodetool - isn't that the same thing as the geometry classes? Wasn't it also committed too late? IIRC it was changed to be able the support the advanced digitizing? And it was only reverted, because it would have needed to be extended in feature freeze to really fix it?

[NathanW2]

I guess the QEP is more of the idea and design but less of the code
(although I can include that if you know what you are doing)

Here is an example:

I would like to show composers and projects as templates with icons like
you have in Word/Excel these days.

• QEP for the idea and designs on the UI. ( I might not know what code I am
  writing at this stage) Assigned to Nyall and someone else to review to
  make sure it fits in with their plans or future plans.
• PR with code changes to implement QEP
• Merge to master if all looks good

I could skip all these steps but then what if my design is crap, and then
it all has to be pulled out and done again or in the worst case which can
happen is someone else has to fix it.

So for me as the project is growing and being used in more professional
environments I think having some process around changes is a good idea
rather then just dropping it in and hoping that everyone agrees, which I am
guilty for.

[3nids]

@jef exactly. I (as map tool maintainer) asked that the tool was switched to click'n'clik to integrate the new features. The requirement was reverted because it was merged without being functionnal at all and close to the release.

[3nids]

I agree that putting common sense into rules it's not that easy. But it's worth trying while we don't have the same "common sense" ;)

[mhugo]

@NathanW2 +1 for the description. And yes, it is more about architecture / design / API

@jef-n

But above there was discussion about excluding small things from this - which obviously makes sense - but of course also makes it unclear to what this QEP would apply

This is why I then propose to make it mandatory for every change, even small ones. But if someone else gives its agreement for the change before the delay, then the PR can be merged directly. It think it's close to what @timlinux proposed.

[wonder-sk]

I completely agree with @jef-n - do we really need this?

I don't think the proposed changes would make any difference to the issues mentioned above. If some dev wants to closely follow the development, he/she can comment on work of others with github comment commits or on mailing lists. Why do we think that making a PR for everything would change anything?

Also, two days for PR review is very short time for a volunteer driven project to review things. Looking at commit stats on github, there is approx a dozen of commits everyday. I have the impression we would just flood ourselves with with useless PRs...

With more and more rules, contributing something is getting more complicated and not fun anymore (which is a problem for any volunteer work). For overwhelming amount of time, this would be just more overhead to bear (e.g. a need to keep lots of branches)

I'm a big fan of sticking to common sense - it worked for us perfectly most of the time for the last X years ... Of course not everyone has the same common sense, but in general it just works :-)

[NathanW2]

I was very open this idea at the start. However after some feelings of
being overwhelmed on the weekend with how much stuff there is now in the
project and not being able to keep up with the pace. I'm going to go with
-1 and say stick to common sense.

If you need someone else to review or are looking for feedback open a PR,
if not push it direct. The QEP process should be able to handle any of the
large changes that need some more up front design and integration.

On Tue, Nov 17, 2015 at 4:40 PM, Martin Dobias notifications@github.com
wrote:

I completely agree with @jef-n https://github.com/jef-n - do we really
need this?

I don't think the proposed changes would make any difference to the issues
mentioned above. If some dev wants to closely follow the development,
he/she can comment on work of others with github comment commits or on
mailing lists. Why do we think that making a PR for everything would change
anything?

Also, two days for PR review is very short time for a volunteer driven
project to review things. Looking at commit stats on github, there is
approx a dozen of commits everyday. I have the impression we would just
flood ourselves with with useless PRs...

With more and more rules, contributing something is getting more
complicated and not fun anymore (which is a problem for any volunteer
work). For overwhelming amount of time, this would be just more overhead to
bear (e.g. a need to keep lots of branches)

I'm a big fan of sticking to common sense - it worked for us perfectly
most of the time for the last X years ... Of course not everyone has the
same common sense, but in general it just works :-)

—
Reply to this email directly or view it on GitHub
#52 (comment)
.

[jef-n]

So I'm not as alone on this as I felt :)

[m-kuhn]

I agree that it is a good idea to put "big" commits into a PR / quarantine first and we should encourage this behavior. But having a rule that forces every commit to go through this will probably flood the PR queue and is an abuse of the reviewer's (volunteer!) time.
I am currently trying to keep up with assigning PR's and giving them a short review. With the proposed system I wouldn't be able to keep up with it.

What do you think about having a non-mandatory system "suggesting to put a bigger change into a quarantine for at least 2 work days before merging it" be acceptable.

Sidenote: I have put the python3 and pyqt5 code into quarantine for a week. It didn't help to identify problems which @jef-n has fixed now. This suggests two things: 1. putting it into a PR is no guarantee to find issues, 2. some core devs already now put things into quarantine on a completely volunteer basis.

[mhugo]

@m-kuhn it sounds reasonable.
My initial intent was also only for "big" changes (or at least non trivial, i.e. that may have impacts). It does not guarantee to find issues, but that's also a way to share information on what is about to be modified.

I agree trying to write down rules is not easy. So, without a new QEP, If we all agree that some delay is recommended for substantial changes before direct merging, I am happy

[mhugo]

And by the way, this new system does not require a review for every PR. That's rather: if someone wants to review a commit before merge, there is a small time window to do so before it's merged. Probably most of the time nobody will react and the change will be merged just after the delay.

[michaelkirk]

If for nothing more than making sure the tests pass this seems like a good idea to me. More than once I've seen cowboy commits by core maintainers break the build.

It's tempting to say let trivial things skip PR's, but it's such a slippery slope, and if people were good judges of when they were about to break something, well I don't think things would break this often.

Asking that every commit goes through PR is not unreasonable.

I do appreciate @m-kuhn's point about it being a burden on volunteers reviewing. I noticed the rails repo uses a fork of high five to automatically assign a reviewer, roulette style. This might be a fair way to help spread the burden.

e.g. rails/rails#22942 new commiter gets a welcoming message, and assigns PR

e.g. from rails/rails#22941 old commiter gets reviewer automatically assigned, but she opted to pass it on.