Re: PR usage by people with commit access

[Mojca Miklavec]

On 5 November 2016 at 08:33, Ryan Schmidt wrote:
>
> Now that we've converted to GitHub, I am automatically receiving emails about 
> each pull request submitted to the MacPorts repositories, and each comment 
> that's made on them. I've tried to participate in some of those, providing 
> feedback on individual lines that are wrong, but it's a lot of work, and 
> often involves follow-up questions from the contributor. And I assume my 
> comments are only seen by the submitter of that pull request and anyone else 
> watching that pull request, and the (I anticipate) very small number of 
> developers watching all pull requests.

The number might not be small (most committers were probably
subscribed), but it will certainly decrease as we get more pull
requests and almost none of the non-committers are subscribed (which
are usually the ones who would benefit from those reviews most).

> So I have the impression that my feedback is reaching fewer viewers and is 
> thus a worse use of my time.

I wonder if  there is a solution for that. (Something like a special
button that says "send this part of the review to the mailing list".
Maybe there's a way to provide some "hook" to send any reviews that
are considered useful to the mailing list automatically, perhaps based
on some special keywoard that we would put to reviews.)

> Since the conversion to GitHub, I'm receiving several hundred emails a day, 
> which is not a quantity that I'm going to be able to keep up with. I'm 
> probably going to unsubscribe from notifications for new pull requests, and 
> continue to provide email feedback based on what gets pushed to the 
> repository and posted to macports-changes.

If you can manage to review all commits (which I always found
extremely impressive), you could also check new pull requests every
now and then even if you are not subscribed and just point out some
most obvious problems before they end up live in the repository.

But I admit that dealing with all the traffic will be tough.

Mojca
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Ryan Schmidt]

> On Nov 4, 2016, at 1:09 PM, Sterling Smith  wrote:
> 
> 
> On Nov 4, 2016, at 10:50AM, Rainer Müller  wrote:
> 
>> On 2016-11-04 18:10, Ivan Larionov wrote:
>> 
>>> * Ability to get a feedback / review from other project members.
>>> 
>>> We use private github setup on my work and we have a rule that you
>>> shouldn't commit directly to master in a project with multiple
>>> contributors until it's very small change like fixing typo. Open PR,
>>> ask for review, merge. Or fix issues and merge if you got any useful
>>> comments on PR.
>> 
>> Who would be better to review the change then the maintainers of ports
>> themselves? I feel like this would unnecessary slow down the process of
>> getting the update out.
>> 
>> Rainer
> 
> In the past, I have seen responses to svn changelogs directed to the 
> committer and copied to the dev list, so apparently port maintainers who are 
> committers are not always the best reviewers.  How many times has there been 
> a post-svn-commit debate about whether something warranted a revision bump?  
> I would recommend that any change that changes the build more than a version 
> and checksum change warrants a pull request.  If no one acts to review it 
> within the timeliness dictated of the committer, then they still have the 
> prerogative and authority to commit the changes when they want. 

I am subscribed to the macports-changes list and eventually try to read every 
commit message, and reply when I have feedback to provide. This reply goes to 
the macports-dev mailing list, where it can benefit all subscribed developers.

I am not subscribed to the macports-tickets list. I think it would be 
overwhelming to receive an email for every new ticket and every comment to 
every ticket. I'm Cc'd on tickets I'm interested in so I'll get emails about 
those, but not about tickets I'm not already involved with.

Now that we've converted to GitHub, I am automatically receiving emails about 
each pull request submitted to the MacPorts repositories, and each comment 
that's made on them. I've tried to participate in some of those, providing 
feedback on individual lines that are wrong, but it's a lot of work, and often 
involves follow-up questions from the contributor. And I assume my comments are 
only seen by the submitter of that pull request and anyone else watching that 
pull request, and the (I anticipate) very small number of developers watching 
all pull requests. So I have the impression that my feedback is reaching fewer 
viewers and is thus a worse use of my time.

Since the conversion to GitHub, I'm receiving several hundred emails a day, 
which is not a quantity that I'm going to be able to keep up with. I'm probably 
going to unsubscribe from notifications for new pull requests, and continue to 
provide email feedback based on what gets pushed to the repository and posted 
to macports-changes.



___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Joshua Root]

On 2016-11-5 06:14 , Daniel J. Luke wrote:

On Nov 4, 2016, at 2:09 PM, Sterling Smith  wrote:

In the past, I have seen responses to svn changelogs directed to the committer 
and copied to the dev list,


I expect that to continue to be the case.


so apparently port maintainers who are committers are not always the best 
reviewers.  How many times has there been a post-svn-commit debate about 
whether something warranted a revision bump?  I would recommend that any change 
that changes the build more than a version and checksum change warrants a pull 
request.  If no one acts to review it within the timeliness dictated of the 
committer, then they still have the prerogative and authority to commit the 
changes when they want.


-1 from me.

I'm not sure what problem this actually would solve, and it would be more work 
for committers.

[if we had lots of people capable and willing to do review every change, then I 
could see it being helpful - but we don't have that]


I can see PRs being helpful in the case where a committer has a change 
to someone else's port ready to go and they just want to run it by the 
maintainer. In that situation it's quicker and easier than the alternatives.


But in general, yeah, not enough hours in the day.

- Josh
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Daniel J. Luke]

On Nov 4, 2016, at 2:09 PM, Sterling Smith  wrote:
> In the past, I have seen responses to svn changelogs directed to the 
> committer and copied to the dev list,

I expect that to continue to be the case.

> so apparently port maintainers who are committers are not always the best 
> reviewers.  How many times has there been a post-svn-commit debate about 
> whether something warranted a revision bump?  I would recommend that any 
> change that changes the build more than a version and checksum change 
> warrants a pull request.  If no one acts to review it within the timeliness 
> dictated of the committer, then they still have the prerogative and authority 
> to commit the changes when they want. 

-1 from me.

I'm not sure what problem this actually would solve, and it would be more work 
for committers.

[if we had lots of people capable and willing to do review every change, then I 
could see it being helpful - but we don't have that]

-- 
Daniel J. Luke



___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Sterling Smith]

On Nov 4, 2016, at 10:50AM, Rainer Müller  wrote:

> On 2016-11-04 18:10, Ivan Larionov wrote:
> 
>> * Ability to get a feedback / review from other project members.
>> 
>> We use private github setup on my work and we have a rule that you
>> shouldn't commit directly to master in a project with multiple
>> contributors until it's very small change like fixing typo. Open PR,
>> ask for review, merge. Or fix issues and merge if you got any useful
>> comments on PR.
> 
> Who would be better to review the change then the maintainers of ports
> themselves? I feel like this would unnecessary slow down the process of
> getting the update out.
> 
> Rainer

In the past, I have seen responses to svn changelogs directed to the committer 
and copied to the dev list, so apparently port maintainers who are committers 
are not always the best reviewers.  How many times has there been a 
post-svn-commit debate about whether something warranted a revision bump?  I 
would recommend that any change that changes the build more than a version and 
checksum change warrants a pull request.  If no one acts to review it within 
the timeliness dictated of the committer, then they still have the prerogative 
and authority to commit the changes when they want. 

My two cents.

-Sterling
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Lawrence Velázquez]

> On Nov 4, 2016, at 1:48 PM, Clemens Lang  wrote:
> 
>> * Using the same change methods as outside contributors may help to
>> develop better PR flow.

I am not particularly interested in accommodating contributors' workflow
expectations for their own sake. Their workflows may work for them but
not for us.

>> Currently I see some lack of the standard flow, maintainers commit
>> contributors' changes in different ways, PRs marked as rejected
>> while they're actually merged, people ask to enable "Allow edits
>> from maintainers" for PR, etc.
> 
> Absolutely agree.

I think this is mainly us trying to figure out a good workflow. I expect
things will settle down eventually.

>> * Ability to get a feedback / review from other project members.
> 
> Agree for the cases where other people interested in the ports and
> capable of reviewing exist, and those people have the time to review in
> a timely fashion.

Sure. I would encourage committers who want feedback to open as many PRs
as they want.

(You get a PR! You get a PR! Everybody gets a PR!!!)

vq
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Rainer Müller]

On 2016-11-04 18:10, Ivan Larionov wrote:
> I'm not saying that you have to wait for review or something like
> this, but opening a PR from your branch and then merging it has some
> pros:
> 
> * Better visibility of changes. Instead of cloning full repository
> and digging through history I can search PRs by port name and see
> changes which were done by maintainers and contributors.

You cannot filter by portname in a reasonable way. GitHub search is just
too limited to support anything like that. That is one of the main
reasons we rejected GitHub Issues, which have the same problem.

Isn't it much easier to just view the history of the corresponding port
directory than a convoluted search? You can also do that on GitHub:

https://github.com/macports/macports-ports/commits/master/python/py-sqlparse

> * Using the same change methods as outside contributors may help to
> develop better PR flow. Currently I see some lack of the standard
> flow, maintainers commit contributors' changes in different ways, PRs
> marked as rejected while they're actually merged, people ask to
> enable "Allow edits from maintainers" for PR, etc.

They were "rejected" because GitHub does not have a way to mark them as
"merged" unless the exact same object was merged. In case the maintainer
had to make additional changes to the commit, GitHub will not recognize
them as merged.

The only solution we found so far is to push the modified commits back
to the pull request branch, which will mark the pull request as
"merged". However, that requires that pull request authors allow
modifications by maintainers.

> * Ability to get a feedback / review from other project members.
> 
> We use private github setup on my work and we have a rule that you
> shouldn't commit directly to master in a project with multiple
> contributors until it's very small change like fixing typo. Open PR,
> ask for review, merge. Or fix issues and merge if you got any useful
> comments on PR.

Who would be better to review the change then the maintainers of ports
themselves? I feel like this would unnecessary slow down the process of
getting the update out.

Rainer
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev

Re: PR usage by people with commit access

[Clemens Lang]

Hi,

On Fri, Nov 04, 2016 at 10:10:44AM -0700, Ivan Larionov wrote:
> I think this is a good practice for port maintainers with write access
> to repository still use PRs instead of direct commits to master.

I agree, with some limitations:

> * Better visibility of changes. Instead of cloning full repository and
> digging through history I can search PRs by port name and see changes
> which were done by maintainers and contributors.

It's more work. We're all doing this in our free time and clicking
through GitHub's Web UI a couple of times just for the searchability is
not worth the extra effort. If you want the full picture, you'll have to
search the history anyway, which luckily is now much faster than it used
to be with Subversion.

> * Using the same change methods as outside contributors may help to
> develop better PR flow. Currently I see some lack of the standard
> flow, maintainers commit contributors' changes in different ways, PRs
> marked as rejected while they're actually merged, people ask to enable
> "Allow edits from maintainers" for PR, etc.

Absolutely agree.

> * Ability to get a feedback / review from other project members.

Agree for the cases where other people interested in the ports and
capable of reviewing exist, and those people have the time to review in
a timely fashion.

> We use private github setup on my work and we have a rule that you
> shouldn't commit directly to master in a project with multiple
> contributors until it's very small change like fixing typo. Open PR,
> ask for review, merge. Or fix issues and merge if you got any useful
> comments on PR.

We do the same thing at my workplace, but this only works because there
are other people with an interest to review your changes (being paid to
do it). I'm not sure whether we have the manpower to review every commit
pushed into MacPorts.

-- 
Clemens
___
macports-dev mailing list
macports-dev@lists.macosforge.org
https://lists.macosforge.org/mailman/listinfo/macports-dev