QEP 52- Commit rule for new feature proposal #52
Comments
NathanW2
changed the title
|
Does this apply to all features though? Does something minor need to go though this process? |
|
Indeed the intent is to avoid the inclusion of important features without notice. |
|
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. |
|
However saying that this is s a +1 from me for this process. |
|
+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. |
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... |
|
@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.
@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 ? |
|
This is what I think would fit best for (not trivial) [FEATURE] requests. For core devs: For not core devs:
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. |
|
@mhugo: The main difference would be that master is never broken (at least from the ci point of view) |
|
@mbernasocchi at least.... until homebrew is down something and causes all the osx tests to fail (like now) ;) |
@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'.
Just to know that all tests pass before code is put in master. |
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. |
also can we make this 2 work days (excluding weekends) |
|
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? |
|
@timlinux @nyalldawson What about:
|
|
What about going on like we ever did? Are there examples for past problems this would have avoided? Is it worth the hassle? |
|
@jef-n because
that's how I see it |
|
examples? |
|
node tool switched back to drag'n'drop |
|
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. |
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. |
|
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.
|
|
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). |
|
@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? |
|
I guess the QEP is more of the idea and design but less of the code Here is an example: I would like to show composers and projects as templates with icons like
I could skip all these steps but then what if my design is crap, and then So for me as the project is growing and being used in more professional |
|
@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. |
|
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" ;) |
|
@NathanW2 +1 for the description. And yes, it is more about architecture / design / API
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. |
|
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 :-) |
|
I was very open this idea at the start. However after some feelings of If you need someone else to review or are looking for feedback open a PR, On Tue, Nov 17, 2015 at 4:40 PM, Martin Dobias [email protected]
|
|
So I'm not as alone on this as I felt :) |
|
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. 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. |
|
@m-kuhn it sounds reasonable. 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 |
|
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. |
|
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. |
NathanW2
changed the title
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:
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
The text was updated successfully, but these errors were encountered: