Pull request policy

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Pull request policy

Nick Hall
Devs,

Should we require pull requests for significant changes?

Paul Culley asked me to add this question to the roadmap:

https://gramps-project.org/wiki/index.php?title=5.1_Roadmap#Pull_requests_for_significant_changes

There have already been some posts about this in the governance thread
and opinions seem to be divided.

The old approach was to discuss proposed changes on the gramps-devel
mailing list before writing code.  After agreement, an existing
developer could merge changes directly into the repository.  New
contributors could become developers after getting a few contributions
reviewed and accepted.  Please read our developer policies for further
details:

https://gramps-project.org/wiki/index.php?title=Developer_policies

The new approach is to use pull requests.  I notice the 57 different
contributors have submitted pull requests so far.

After considering the feedback, I suggest we keep both approaches for
now.  Direct merging will still be acceptable for minor changes.  Since
this only applies to senior developers, I will leave the definition of
"minor" up to the individual developer.

All major changes, including any change to the user interface, or a
change that requires a code review, should be made by submitting a pull
request.  Proposed changes should still be discussed on the mailing list
first.  I will be using pull requests for all my changes in the future.

We still need to improve our pull request policy and answer questions
such as:

Should discussion move from the mailing list to the PR?

Should the submitter be allowed to merge the PR?

How long should a PR remain open for reviews, comments and testing
before merging?

I look forward to reading your opinions.

Regards,


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Luigi Toscano
Nick Hall ha scritto:

> After considering the feedback, I suggest we keep both approaches for now. 
> Direct merging will still be acceptable for minor changes.  Since this only
> applies to senior developers, I will leave the definition of "minor" up to the
> individual developer.
>
> All major changes, including any change to the user interface, or a change
> that requires a code review, should be made by submitting a pull request. 
> Proposed changes should still be discussed on the mailing list first.  I will
> be using pull requests for all my changes in the future.

Can translation-only changes still be pushed directly?

Ciao
--
Luigi

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Nick Hall
On 19/04/18 16:28, Luigi Toscano wrote:
> Can translation-only changes still be pushed directly?

Yes.

Translation updates would be considered a minor change.  They generally
only update a single file, and there are fewer people able to review them.

Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

John Ralls-2
In reply to this post by Nick Hall
First try bounced from SourceForge, trying again with a different smtp server:

On Apr 19, 2018, at 7:59 AM, Nick Hall <[hidden email]> wrote:

Devs,

Should we require pull requests for significant changes?

Paul Culley asked me to add this question to the roadmap:

https://gramps-project.org/wiki/index.php?title=5.1_Roadmap#Pull_requests_for_significant_changes

There have already been some posts about this in the governance thread and opinions seem to be divided.

The old approach was to discuss proposed changes on the gramps-devel mailing list before writing code.  After agreement, an existing developer could merge changes directly into the repository.  New contributors could become developers after getting a few contributions reviewed and accepted.  Please read our developer policies for further details:

https://gramps-project.org/wiki/index.php?title=Developer_policies

The new approach is to use pull requests.  I notice the 57 different contributors have submitted pull requests so far.

After considering the feedback, I suggest we keep both approaches for now.  Direct merging will still be acceptable for minor changes.  Since this only applies to senior developers, I will leave the definition of "minor" up to the individual developer.

All major changes, including any change to the user interface, or a change that requires a code review, should be made by submitting a pull request.  Proposed changes should still be discussed on the mailing list first.  I will be using pull requests for all my changes in the future.

We still need to improve our pull request policy and answer questions such as:

Should discussion move from the mailing list to the PR?

Should the submitter be allowed to merge the PR?

How long should a PR remain open for reviews, comments and testing before merging?

I look forward to reading your opinions.

Nick,

My understanding of current policy is that major changes or new features should be written up as a GEP on the wiki and discussed there before beginning to code. That’s absent from the Developer policies page. Also not mentioned anywhere is the importance of reviewing and discussing GEPs and the need for the GEP proposer to recruit as much discussion as possible among both users and developers. Your position as Architect means that you need to both actively participate in those discussions and help recruit others to the discussion.

Any significant design change or new feature should always be discussed thoroughly among the team before beginning to write code. First the scope of the change or new feature, then the general implementation approach should be sketched out. A pull request is way too late for those kind of discussions: The author has already made the important decisions and exerted considerable effort to putting them in code. If at that point there’s disagreement with the rest of the team it turns into a fight and the results are ugly: We just went through that with Doug Blank and DBAPI. There were some other leadership factors at play, but more discussion and deeper analysis by the rest of the team could have prevented some of the acrimony and gotten to a useable implementation more quickly.

That needs to be made very clear to potential outside contributors as well. It’s always painful to tell someone who’s submitted a several-hundred-line PR “no” because they want to take the project in the wrong direction or their design or decomposition is poor. I’ve never once seen a case where the person took the criticism on board and started over with a discussion of how to get what he wants into the program.

None the less, everyone benefits from getting their code reviewed early and often, even professors of computer science. We all make mistakes and we all make poor design choices and earlier and more frequent reviews mean smaller “course corrections” that in turn means much less friction. By the time a big change is offered as a PR it should be a matter of final proof-reading with no surprises for anyone on the team.

There’s a grey zone somewhere between “fixing a typo in README” and “significant design change” where the need for a PR and a little further up the line the need for a thorough design discussion aren’t necessary. The only way to avoid individual judgement on where a particular change falls in that grey zone is to decree that it doesn’t exist: Even fixing a typo requires prior discussion and a PR. That’s clearly a time-waster so we need to either reach some consensus or you need to decree where and how wide are those grey zones.

Regards,
John Ralls
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Nick Hall
In reply to this post by Nick Hall
Yet another missing post.  So in reply to John:

It looks like we need to write a contribution policy to make the process
clear to everyone.  At the moment we appear to be missing two important
stages:  discussion on the mailing list where the scope of a change and
general implementation approach can be agreed prior to coding and code
review.

I'll start with a suggestion for a new contribution policy.

For any significant design change or new feature:

1. Create a feature request or GEPS for the change.
2. Discuss the scope and general implementation approach on the
gramps-devel mailing list.  A GEPS may be requested if one has not
already been written.
3. Add the change to the roadmap if approved.
4. Create a pull request and allow the code to be reviewed.
5. Merge the pull request.

For bug fixes:

1. Create a bug report for the change.
2. Discuss the scope and general implementation approach on the mailing
list.
3. Create a pull request and allow the code to be reviewed.
4. Merge the pull request.

I agree that requiring pull requests for some changes would be a waste
of time.  Such changes would include:  the release process, updates to
translations and trivial fixes.

I would prefer if we could reach a consensus for the new policy.

Regards,


Nick.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Nick Hall
On 19/04/18 22:48, Nick Hall wrote:
> It looks like we need to write a contribution policy to make the
> process clear to everyone.  At the moment we appear to be missing two
> important stages:  discussion on the mailing list where the scope of a
> change and general implementation approach can be agreed prior to
> coding and code review.

Let me highlight our existing policy for adding new features:

1. Create a feature request.

2. Discuss the change on the list.   This step is not optional.

3. Write a GEPS if requested.

4. Agree the design on the list.

5. Create a GEPS branch for large changes.

6. Code the change.

7. Either push the change directly if you have write access to the
repository, or attach a patch to the FR for review.

I have just reverted a couple of commits because they contained changes
that were not discussed on the list and changes I requested in the PRs
were not made.  This is probably partly due to the fact that our
contribution policy needs updating.

Until a new contribution policy is agreed, discussing new features on
the list is essential. For now, PRs are just an easy way to submit a patch.

At the moment, the primary method of communication for developers is on
the list.  Do we want to move more of the discussion into PRs? Does
anyone else have any other comments about a new contribution policy?

Regards,


Nick.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

John Ralls-2


> On May 12, 2018, at 4:57 PM, Nick Hall <[hidden email]> wrote:
>
> On 19/04/18 22:48, Nick Hall wrote:
>> It looks like we need to write a contribution policy to make the process clear to everyone.  At the moment we appear to be missing two important stages:  discussion on the mailing list where the scope of a change and general implementation approach can be agreed prior to coding and code review.
>
> Let me highlight our existing policy for adding new features:
>
> 1. Create a feature request.
>
> 2. Discuss the change on the list.   This step is not optional.
>
> 3. Write a GEPS if requested.
>
> 4. Agree the design on the list.
>
> 5. Create a GEPS branch for large changes.
>
> 6. Code the change.
>
> 7. Either push the change directly if you have write access to the repository, or attach a patch to the FR for review.
>
> I have just reverted a couple of commits because they contained changes that were not discussed on the list and changes I requested in the PRs were not made.  This is probably partly due to the fact that our contribution policy needs updating.
>
> Until a new contribution policy is agreed, discussing new features on the list is essential. For now, PRs are just an easy way to submit a patch.
>
> At the moment, the primary method of communication for developers is on the list.  Do we want to move more of the discussion into PRs? Does anyone else have any other comments about a new contribution policy?
>

6.5: Unless it’s a trivial change, get a code review. One easy way is to make a PR, but one can also push to a branch on one’s personal Github repo and ask here for other devs to have a look.

We all make mistakes and occasionally go off in the wrong direction and getting another developer to look things over is a good way to avoid dumb mistakes.

Regards,
John Ralls


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Benny Malengier
I believe you should add also the alternative way of starting from a ticket/issue.  Then first discussion happens there and can already be quite elaborate. Developer starts a branch for the fix, then first commit should contain link to the ticket.
Out of this, a PR can follow.
However, it can become clear more input is needed to fix the ticket, then discussion moves to developer list, ...

Op zo 13 mei 2018 om 05:05 schreef John Ralls <[hidden email]>:


> On May 12, 2018, at 4:57 PM, Nick Hall <[hidden email]> wrote:
>
> On 19/04/18 22:48, Nick Hall wrote:
>> It looks like we need to write a contribution policy to make the process clear to everyone.  At the moment we appear to be missing two important stages:  discussion on the mailing list where the scope of a change and general implementation approach can be agreed prior to coding and code review.
>
> Let me highlight our existing policy for adding new features:
>
> 1. Create a feature request.
>
> 2. Discuss the change on the list.   This step is not optional.
>
> 3. Write a GEPS if requested.
>
> 4. Agree the design on the list.
>
> 5. Create a GEPS branch for large changes.
>
> 6. Code the change.
>
> 7. Either push the change directly if you have write access to the repository, or attach a patch to the FR for review.
>
> I have just reverted a couple of commits because they contained changes that were not discussed on the list and changes I requested in the PRs were not made.  This is probably partly due to the fact that our contribution policy needs updating.
>
> Until a new contribution policy is agreed, discussing new features on the list is essential. For now, PRs are just an easy way to submit a patch.
>
> At the moment, the primary method of communication for developers is on the list.  Do we want to move more of the discussion into PRs? Does anyone else have any other comments about a new contribution policy?
>

6.5: Unless it’s a trivial change, get a code review. One easy way is to make a PR, but one can also push to a branch on one’s personal Github repo and ask here for other devs to have a look.

We all make mistakes and occasionally go off in the wrong direction and getting another developer to look things over is a good way to avoid dumb mistakes.

Regards,
John Ralls


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Nick Hall
On 15/05/18 17:09, Benny Malengier wrote:
> I believe you should add also the alternative way of starting from a
> ticket/issue.  Then first discussion happens there and can already be
> quite elaborate. Developer starts a branch for the fix, then first
> commit should contain link to the ticket.
> Out of this, a PR can follow.
> However, it can become clear more input is needed to fix the ticket,
> then discussion moves to developer list, ...

Thanks.

I have added a new "Contributing" section to our committing policies.

https://gramps-project.org/wiki/index.php?title=Committing_policies#Contributing

It is just a start.  Please give feedback so that we can refine it.

Regards,


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Benny Malengier
Automatic bug links in the git tree would be easier (add #3241 to commit message to link to issue 3241 and have in the issue appear it was mentioned in a commit).
 
Moving to github issues could provide this. With Mantis, perhaps interesting to test https://noswap.com/projects/source-integration as it seems to be able to do the same with mantis.

Benny

Op wo 16 mei 2018 om 00:36 schreef Nick Hall <[hidden email]>:
On 15/05/18 17:09, Benny Malengier wrote:
> I believe you should add also the alternative way of starting from a
> ticket/issue.  Then first discussion happens there and can already be
> quite elaborate. Developer starts a branch for the fix, then first
> commit should contain link to the ticket.
> Out of this, a PR can follow.
> However, it can become clear more input is needed to fix the ticket,
> then discussion moves to developer list, ...

Thanks.

I have added a new "Contributing" section to our committing policies.

https://gramps-project.org/wiki/index.php?title=Committing_policies#Contributing

It is just a start.  Please give feedback so that we can refine it.

Regards,


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|

Re: Pull request policy

Nick Hall
We enabled the Mantis GitHub source integration on 3 Jun 2017.  See:

https://sourceforge.net/p/gramps/mailman/message/35875721/

It works quite well, but we do get the occasional webhook timeout.

Nick.


On 17/05/18 09:48, Benny Malengier wrote:
> Automatic bug links in the git tree would be easier (add #3241 to
> commit message to link to issue 3241 and have in the issue appear it
> was mentioned in a commit).
>
> Moving to github issues could provide this. With Mantis, perhaps
> interesting to test https://noswap.com/projects/source-integration as
> it seems to be able to do the same with mantis.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel