Github in-web editing: disable?

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

Github in-web editing: disable?

jamshark70-2
I wonder if there is a way to remove the edit button from github's file
view, so that all changes must go through pull requests that the
contributor first makes on her local machine.

I took a quick spin through github permissions. The only thing I can find
is to set the repository's default permission to "read" rather than
"write." I'm not sure what that will do for the little pencil icon, though.

I have a few reasons:

1. Mainly, I think it's reasonable that all changes should be tested
locally before being pushed, even into a topic branch. It's too easy to
open up a file in github's web view, click "edit," mess up some syntax and
commit. Then it becomes a senior developer's responsibility to apply the
change locally and test. I think that the burden of basic, first-run
testing should be spread around. If you have no choice but to make the
change locally first, it's a short step toward testing it locally. (Even
helpfile changes... it's exactly the moment when you think, "ah, I'm just
changing a couple of words, it's fine" that you bumped the delete key
accidentally and broke something without noticing. ALL changes need at
least a basic test before leaving one's local machine. ALL of them.)

2. As a member of the SC organization, I can edit on the web and push
directly to master (!). That's a little dangerous. I'm not sure if
non-members have the same privilege (but I'm guessing not, otherwise we'd
have a bigger mess than the dozens of single-commit PRs that came through
recently). But even as a longtime user in good standing, I'm not sure even
*I* want the power to mess things up casually ;)

3. Github web fills in a default commit message "Update [filename]" instead
of requiring the user to enter a proper commit message before submitting.
There is nothing to discourage users from putting in large numbers of
small, poorly identified commits, making it harder for admins to evaluate
and prioritize.

I don't think it should be a requirement until our github best practices
document is complete, but I do think it's reasonable to steer users toward
proper branches, grouping of commits, etc.

hjh

Sent with AquaMail for Android
http://www.aqua-mail.com




_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

brianlheim
All good ideas, although I think the one time it's easier to edit in-browser is with markdown files. Not a very good reason, but just wanted to point that out.

This option is currently disabled for master. Would anyone be opposed to enabling it, for administrators as well as all others? It sounds exactly like what you're suggesting, and also is good practice, and I see no reason not to enable it as a failsafe.

`Require pull request reviews before merging`
`When enabled, all commits must be made to a non-protected branch and submitted via a pull request with at least one approved review and no changes requested before it can be merged into master.`

Regards,

Brian


On Tue, Feb 28, 2017 at 6:57 PM, James Harkins <[hidden email]> wrote:
I wonder if there is a way to remove the edit button from github's file view, so that all changes must go through pull requests that the contributor first makes on her local machine.

I took a quick spin through github permissions. The only thing I can find is to set the repository's default permission to "read" rather than "write." I'm not sure what that will do for the little pencil icon, though.

I have a few reasons:

1. Mainly, I think it's reasonable that all changes should be tested locally before being pushed, even into a topic branch. It's too easy to open up a file in github's web view, click "edit," mess up some syntax and commit. Then it becomes a senior developer's responsibility to apply the change locally and test. I think that the burden of basic, first-run testing should be spread around. If you have no choice but to make the change locally first, it's a short step toward testing it locally. (Even helpfile changes... it's exactly the moment when you think, "ah, I'm just changing a couple of words, it's fine" that you bumped the delete key accidentally and broke something without noticing. ALL changes need at least a basic test before leaving one's local machine. ALL of them.)

2. As a member of the SC organization, I can edit on the web and push directly to master (!). That's a little dangerous. I'm not sure if non-members have the same privilege (but I'm guessing not, otherwise we'd have a bigger mess than the dozens of single-commit PRs that came through recently). But even as a longtime user in good standing, I'm not sure even *I* want the power to mess things up casually ;)

3. Github web fills in a default commit message "Update [filename]" instead of requiring the user to enter a proper commit message before submitting. There is nothing to discourage users from putting in large numbers of small, poorly identified commits, making it harder for admins to evaluate and prioritize.

I don't think it should be a requirement until our github best practices document is complete, but I do think it's reasonable to steer users toward proper branches, grouping of commits, etc.

hjh

Sent with AquaMail for Android
http://www.aqua-mail.com




_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/



--
_______________________________
Brian Heim
507-429-6468

B.M. '14 University of Texas at Austin
M.M. '16 Yale School of Music
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

julian.rohrhuber
Hi Brian and James,

it would be nice if there was some granularity for this, but probably this is not the case. Currently the hurdle is still quite high for casually contributing to sc. In particular having to make a branch for each little change is not the best features of the git workflow.

And what’s more, so many little text improvements could come in if this weren’t so complicated. I always liked the spirit of not separating between users and developers.

Also, I’m just saying this as a general comment. I find it very reasonable that go through a review process.

Regards,
Julian

> On 01.03.2017, at 21:41, brian heim <[hidden email]> wrote:
>
> All good ideas, although I think the one time it's easier to edit in-browser is with markdown files. Not a very good reason, but just wanted to point that out.
>
> This option is currently disabled for master. Would anyone be opposed to enabling it, for administrators as well as all others? It sounds exactly like what you're suggesting, and also is good practice, and I see no reason not to enable it as a failsafe.
>
> `Require pull request reviews before merging`
> `When enabled, all commits must be made to a non-protected branch and submitted via a pull request with at least one approved review and no changes requested before it can be merged into master.`
>
> Regards,
>
> Brian
>
>
> On Tue, Feb 28, 2017 at 6:57 PM, James Harkins <[hidden email]> wrote:
> I wonder if there is a way to remove the edit button from github's file view, so that all changes must go through pull requests that the contributor first makes on her local machine.
>
> I took a quick spin through github permissions. The only thing I can find is to set the repository's default permission to "read" rather than "write." I'm not sure what that will do for the little pencil icon, though.
>
> I have a few reasons:
>
> 1. Mainly, I think it's reasonable that all changes should be tested locally before being pushed, even into a topic branch. It's too easy to open up a file in github's web view, click "edit," mess up some syntax and commit. Then it becomes a senior developer's responsibility to apply the change locally and test. I think that the burden of basic, first-run testing should be spread around. If you have no choice but to make the change locally first, it's a short step toward testing it locally. (Even helpfile changes... it's exactly the moment when you think, "ah, I'm just changing a couple of words, it's fine" that you bumped the delete key accidentally and broke something without noticing. ALL changes need at least a basic test before leaving one's local machine. ALL of them.)
>
> 2. As a member of the SC organization, I can edit on the web and push directly to master (!). That's a little dangerous. I'm not sure if non-members have the same privilege (but I'm guessing not, otherwise we'd have a bigger mess than the dozens of single-commit PRs that came through recently). But even as a longtime user in good standing, I'm not sure even *I* want the power to mess things up casually ;)
>
> 3. Github web fills in a default commit message "Update [filename]" instead of requiring the user to enter a proper commit message before submitting. There is nothing to discourage users from putting in large numbers of small, poorly identified commits, making it harder for admins to evaluate and prioritize.
>
> I don't think it should be a requirement until our github best practices document is complete, but I do think it's reasonable to steer users toward proper branches, grouping of commits, etc.
>
> hjh
>
> Sent with AquaMail for Android
> http://www.aqua-mail.com
>
>
>
>
> _______________________________________________
> sc-dev mailing list
>
> info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
> archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
> search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
>
>
>
> --
> _______________________________
> Brian Heim
> 507-429-6468
> www.brianlheim.com
>
> B.M. '14 University of Texas at Austin
> M.M. '16 Yale School of Music


_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

Rainer Schuetz
Hi all,

I am with the "make contributing easy" and "no enforced distinction between users and developers" side of this discussion. We still need more contribut(ors/ions) and intuitive ways to join the git workflow. It becomes natural after a while, but it is a total show stopper before you've been baptized. One tends to forget that after a while. Some elements in our workflow are still terribly complicated/intransparent (contributing to a submodule and getting the change into master, co-contributing to PRs). Note that using the Webinterface gives you a hint at what forking and branching is, by doing it for you, and telling you about it. Of course non-admins cannot merge anything.

I can see what triggered this request into yet more formality that is then ignored. But the actual process is evolutionary, attempts at intelligent design have failed over generations ;) I think the better way to reduce such incidents is to actively promote alternative ways to join discourse around SC (which are also contributions). Being less controlling might need some patience at times, but I believe it will pay in form of a more active community on the long run. The nice side of the emergence of so many fora on SC is that everybody can ultimately get what she is after.

Best
.r.


> On 2 Mar 2017, at 10:04, Julian Rohrhuber <[hidden email]> wrote:
>
> Hi Brian and James,
>
> it would be nice if there was some granularity for this, but probably this is not the case. Currently the hurdle is still quite high for casually contributing to sc. In particular having to make a branch for each little change is not the best features of the git workflow.
>
> And what’s more, so many little text improvements could come in if this weren’t so complicated. I always liked the spirit of not separating between users and developers.
>
> Also, I’m just saying this as a general comment. I find it very reasonable that go through a review process.
>
> Regards,
> Julian
>
>> On 01.03.2017, at 21:41, brian heim <[hidden email]> wrote:
>>
>> All good ideas, although I think the one time it's easier to edit in-browser is with markdown files. Not a very good reason, but just wanted to point that out.
>>
>> This option is currently disabled for master. Would anyone be opposed to enabling it, for administrators as well as all others? It sounds exactly like what you're suggesting, and also is good practice, and I see no reason not to enable it as a failsafe.
>>
>> `Require pull request reviews before merging`
>> `When enabled, all commits must be made to a non-protected branch and submitted via a pull request with at least one approved review and no changes requested before it can be merged into master.`
>>
>> Regards,
>>
>> Brian
>>
>>
>> On Tue, Feb 28, 2017 at 6:57 PM, James Harkins <[hidden email]> wrote:
>> I wonder if there is a way to remove the edit button from github's file view, so that all changes must go through pull requests that the contributor first makes on her local machine.
>>
>> I took a quick spin through github permissions. The only thing I can find is to set the repository's default permission to "read" rather than "write." I'm not sure what that will do for the little pencil icon, though.
>>
>> I have a few reasons:
>>
>> 1. Mainly, I think it's reasonable that all changes should be tested locally before being pushed, even into a topic branch. It's too easy to open up a file in github's web view, click "edit," mess up some syntax and commit. Then it becomes a senior developer's responsibility to apply the change locally and test. I think that the burden of basic, first-run testing should be spread around. If you have no choice but to make the change locally first, it's a short step toward testing it locally. (Even helpfile changes... it's exactly the moment when you think, "ah, I'm just changing a couple of words, it's fine" that you bumped the delete key accidentally and broke something without noticing. ALL changes need at least a basic test before leaving one's local machine. ALL of them.)
>>
>> 2. As a member of the SC organization, I can edit on the web and push directly to master (!). That's a little dangerous. I'm not sure if non-members have the same privilege (but I'm guessing not, otherwise we'd have a bigger mess than the dozens of single-commit PRs that came through recently). But even as a longtime user in good standing, I'm not sure even *I* want the power to mess things up casually ;)
>>
>> 3. Github web fills in a default commit message "Update [filename]" instead of requiring the user to enter a proper commit message before submitting. There is nothing to discourage users from putting in large numbers of small, poorly identified commits, making it harder for admins to evaluate and prioritize.
>>
>> I don't think it should be a requirement until our github best practices document is complete, but I do think it's reasonable to steer users toward proper branches, grouping of commits, etc.
>>
>> hjh
>>
>> Sent with AquaMail for Android
>> http://www.aqua-mail.com
>>
>>
>>
>>
>> _______________________________________________
>> sc-dev mailing list
>>
>> info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
>> archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
>> search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
>>
>>
>>
>> --
>> _______________________________
>> Brian Heim
>> 507-429-6468
>> www.brianlheim.com
>>
>> B.M. '14 University of Texas at Austin
>> M.M. '16 Yale School of Music
>
>
> _______________________________________________
> sc-dev mailing list
>
> info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
> archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
> search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/


_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

brianlheim
It's fine to discuss this topic, but none of this really applies to the option I shared. Requiring a review approval before merging is a trivial change to the workflow--just click 'approved' before you merge. The rest is behavior we already follow.

-Brian
 
On Thu, Mar 2, 2017 at 6:02 AM Rainer Schuetz <[hidden email]> wrote:
Hi all,

I am with the "make contributing easy" and "no enforced distinction between users and developers" side of this discussion. We still need more contribut(ors/ions) and intuitive ways to join the git workflow. It becomes natural after a while, but it is a total show stopper before you've been baptized. One tends to forget that after a while. Some elements in our workflow are still terribly complicated/intransparent (contributing to a submodule and getting the change into master, co-contributing to PRs). Note that using the Webinterface gives you a hint at what forking and branching is, by doing it for you, and telling you about it. Of course non-admins cannot merge anything.

I can see what triggered this request into yet more formality that is then ignored. But the actual process is evolutionary, attempts at intelligent design have failed over generations ;) I think the better way to reduce such incidents is to actively promote alternative ways to join discourse around SC (which are also contributions). Being less controlling might need some patience at times, but I believe it will pay in form of a more active community on the long run. The nice side of the emergence of so many fora on SC is that everybody can ultimately get what she is after.

Best
.r.


> On 2 Mar 2017, at 10:04, Julian Rohrhuber <[hidden email]> wrote:
>
> Hi Brian and James,
>
> it would be nice if there was some granularity for this, but probably this is not the case. Currently the hurdle is still quite high for casually contributing to sc. In particular having to make a branch for each little change is not the best features of the git workflow.
>
> And what’s more, so many little text improvements could come in if this weren’t so complicated. I always liked the spirit of not separating between users and developers.
>
> Also, I’m just saying this as a general comment. I find it very reasonable that go through a review process.
>
> Regards,
> Julian
>
>> On 01.03.2017, at 21:41, brian heim <[hidden email]> wrote:
>>
>> All good ideas, although I think the one time it's easier to edit in-browser is with markdown files. Not a very good reason, but just wanted to point that out.
>>
>> This option is currently disabled for master. Would anyone be opposed to enabling it, for administrators as well as all others? It sounds exactly like what you're suggesting, and also is good practice, and I see no reason not to enable it as a failsafe.
>>
>> `Require pull request reviews before merging`
>> `When enabled, all commits must be made to a non-protected branch and submitted via a pull request with at least one approved review and no changes requested before it can be merged into master.`
>>
>> Regards,
>>
>> Brian
>>
>>
>> On Tue, Feb 28, 2017 at 6:57 PM, James Harkins <[hidden email]> wrote:
>> I wonder if there is a way to remove the edit button from github's file view, so that all changes must go through pull requests that the contributor first makes on her local machine.
>>
>> I took a quick spin through github permissions. The only thing I can find is to set the repository's default permission to "read" rather than "write." I'm not sure what that will do for the little pencil icon, though.
>>
>> I have a few reasons:
>>
>> 1. Mainly, I think it's reasonable that all changes should be tested locally before being pushed, even into a topic branch. It's too easy to open up a file in github's web view, click "edit," mess up some syntax and commit. Then it becomes a senior developer's responsibility to apply the change locally and test. I think that the burden of basic, first-run testing should be spread around. If you have no choice but to make the change locally first, it's a short step toward testing it locally. (Even helpfile changes... it's exactly the moment when you think, "ah, I'm just changing a couple of words, it's fine" that you bumped the delete key accidentally and broke something without noticing. ALL changes need at least a basic test before leaving one's local machine. ALL of them.)
>>
>> 2. As a member of the SC organization, I can edit on the web and push directly to master (!). That's a little dangerous. I'm not sure if non-members have the same privilege (but I'm guessing not, otherwise we'd have a bigger mess than the dozens of single-commit PRs that came through recently). But even as a longtime user in good standing, I'm not sure even *I* want the power to mess things up casually ;)
>>
>> 3. Github web fills in a default commit message "Update [filename]" instead of requiring the user to enter a proper commit message before submitting. There is nothing to discourage users from putting in large numbers of small, poorly identified commits, making it harder for admins to evaluate and prioritize.
>>
>> I don't think it should be a requirement until our github best practices document is complete, but I do think it's reasonable to steer users toward proper branches, grouping of commits, etc.
>>
>> hjh
>>
>> Sent with AquaMail for Android
>> http://www.aqua-mail.com
>>
>>
>>
>>
>> _______________________________________________
>> sc-dev mailing list
>>
>> info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
>> archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
>> search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
>>
>>
>>
>> --
>> _______________________________
>> Brian Heim
>> 507-429-6468
>> www.brianlheim.com
>>
>> B.M. '14 University of Texas at Austin
>> M.M. '16 Yale School of Music
>
>
> _______________________________________________
> sc-dev mailing list
>
> info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
> archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
> search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/


_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
--
_______________________________
Brian Heim
507-429-6468

B.M. '14 University of Texas at Austin
M.M. '16 Yale School of Music
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

Rainer Schuetz

On 2 Mar 2017, at 14:24, brian heim <[hidden email]> wrote:

It's fine to discuss this topic, but none of this really applies to the option I shared.

Sure, I wasn't replying to you specifically.

Requiring a review approval before merging is a trivial change to the workflow--just click 'approved' before you merge. The rest is behavior we already follow.

Yes, and I am not particularly passionate about this one. But if you want to be subtle: this it means that (at least in theory) three independently acting individuals have to take responsibility for a PR (and two times that if a submodule AND master are involved). In some domains of SC development (e.g. scvim-module, build system, Qt-stuff) this is already too much and puts PRs in an endless waiting queue where it will ultimately stale. At the same time the PR queue grows and contributors get frustrated, and people complain about (often trivial) usability issues in fora not linked to the community controlling the sc code. I've seen people role their eyes when you mention SC ;). I continue to believe that self responsibility of contributors is a factor to be taken into account. The determination to follow up on ones contributions is worth more than elaborate control structures. I've seen people in this forum insist on formality and then remain quite when their contributions that passed the formal thresholds turned out to be buggy or problematic in an unforeseen way. Also accidents can usually easily be undone. You just have to accept a bit of dirt in the commit history. My 2 cents.

Best
.r.

Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

brianlheim
> this it means that (at least in theory) three independently acting individuals have to take responsibility for a PR

I think you misunderstood—the same admin is allowed to both review and merge. There is no other workflow difference aside from checking a box that we already check half the time anyway. And I think that protecting master from accidental commits even from admins is worth this slight change.

There are a few options GitHub offers for protected branches:

- Require PR reviews before merging (off)
- Require status checks to pass (on)
- Restrict who can push to this branch (off)

For a repo of this size and importance, I'm kind of surprised that the last one is turned off. I think it would be best practice to turn all of them on. Again, no added hurdles, just solidifying and guarding the practices we already follow.

-Brian

On Thu, Mar 2, 2017 at 8:50 AM, Rainer Schuetz <[hidden email]> wrote:

On 2 Mar 2017, at 14:24, brian heim <[hidden email]> wrote:

It's fine to discuss this topic, but none of this really applies to the option I shared.

Sure, I wasn't replying to you specifically.

Requiring a review approval before merging is a trivial change to the workflow--just click 'approved' before you merge. The rest is behavior we already follow.

Yes, and I am not particularly passionate about this one. But if you want to be subtle: this it means that (at least in theory) three independently acting individuals have to take responsibility for a PR (and two times that if a submodule AND master are involved). In some domains of SC development (e.g. scvim-module, build system, Qt-stuff) this is already too much and puts PRs in an endless waiting queue where it will ultimately stale. At the same time the PR queue grows and contributors get frustrated, and people complain about (often trivial) usability issues in fora not linked to the community controlling the sc code. I've seen people role their eyes when you mention SC ;). I continue to believe that self responsibility of contributors is a factor to be taken into account. The determination to follow up on ones contributions is worth more than elaborate control structures. I've seen people in this forum insist on formality and then remain quite when their contributions that passed the formal thresholds turned out to be buggy or problematic in an unforeseen way. Also accidents can usually easily be undone. You just have to accept a bit of dirt in the commit history. My 2 cents.

Best
.r.




--
_______________________________
Brian Heim
507-429-6468

B.M. '14 University of Texas at Austin
M.M. '16 Yale School of Music
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

Rainer Schuetz

On 2 Mar 2017, at 17:40, brian heim <[hidden email]> wrote:

> this it means that (at least in theory) three independently acting individuals have to take responsibility for a PR

I think you misunderstood

Why? A single person could make a PR, approve it and commit it, but that is not in the spirit of the checks (therefore I added "in theory"). You approve something kind of implies you don't commit it, even if you could. That is good only as long as we have enough woman-power to treat contributions well. But as I said, I don't think this is a big issue.

I am against adding more steps and further tightening control. I hope a decision making process about such changes makes sure it has heard many voices.

Best
.r.

Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

jamshark70-2
In reply to this post by Rainer Schuetz
---- On Thu, 02 Mar 2017 19:01:49 +0800 Rainer Schuetz <[hidden email]> wrote ----

 > I am with the "make contributing easy" and "no enforced distinction between users and developers" side of this discussion. We still need more contribut(ors/ions) and intuitive ways to join the git workflow. It becomes natural after a while, but it is a total show stopper before you've been baptized. One tends to forget that after a while. Some elements in our workflow are still terribly complicated/intransparent (contributing to a submodule and getting the change into master, co-contributing to PRs). Note that using the Webinterface gives you a hint at what forking and branching is, by doing it for you, and telling you about it. Of course non-admins cannot merge anything.
 >  
 > I can see what triggered this request into yet more formality that is then ignored. But the actual process is evolutionary, attempts at intelligent design have failed over generations ;) I think the better way to reduce such incidents is to actively promote alternative ways to join discourse around SC (which are also contributions). Being less controlling might need some patience at times, but I believe it will pay in form of a more active community on the long run. The nice side of the emergence of so many fora on SC is that everybody can ultimately get what she is after.

In general, I don't disagree that it's better to keep the entry level for contributing as low as possible without breaking things (and I'll admit to being a bit polemical in my first post). A low bar does assume good faith. Admittedly, it isn't a *huge* problem yet, but we do have a situation arising periodically in which one member Really Really Wants to contribute something -- anything -- but this member also has not taken advice from the community to heart, and persists in submitting PRs of the sort that the community has already said we don't want. I suppose, in principle, tolerance and patience are the best approach. But, when I wake up for each of a few consecutive days and find 80-90 e-mails about this person's last batch of 20 PRs, sure, it tests my patience (partly because the PRs themselves, though well-intentioned, show a lack of respect for developers' time).

About reviews, ideally, they would happen more quickly rather than relaxing standards. You mentioned the case of unforeseen problems with a change. Actually, to me, that's understandable -- I'm a mediocre coder and a worse tester, and I'm perfectly willing to cut slack for not testing everything. That said... the cases that drive me nuts are the ones where something was committed and passed review, but even the most fundamental test case failed -- giving the appearance of a thought process like, "eh, this *ought* to work... push it." I think we can reasonably discourage that approach ;)

hjh


_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

brianlheim
OK, I was a little confused about the meaning of the options after today's meeting, so I just tried out some of the protected branch options with a separate non-admin GH account and some of my own repos.

Repeating what James has already said in his first post, it's rather dangerous that those with write-access can push non-merge commits directly to master. We develop using master already, and (as far as I've observed) we have a number of users who frequently build from it. If we mess that up, it's going to look really unprofessional.

The `Require pull request reviews before merging` option will disable this possibility if we turn it on. The one caveat is that it requires PRs to have at least one approved review before being merged. The approving admin and the admin who merges may be the same person. I would argue that having this is a bonus, as it helps to formalize our current review process (try to have at least two other people look at a PR, one in the case of text changes) as agreed on in earlier dev meetings, and will also encourage us to make our PRs more easily reviewable.

In addition, we can also choose whether or not to enforce these requirements for PRs made by admins. In other words, we could have it so that PRs by admins wouldn't require a review; PRs by others would. I'd lean more toward enforcing it for everyone, but happy with either option as long as it means none of us is able to accidentally push to master.

The `restrict who can push to this branch` is almost meaningless right now, because the only people who can push are admins and those on the "for notifications" team. I don't see a real reason to restrict that to only admins—the roles of the various teams are not well-defined anyway, and it would unjustifiably shut out a few people that are on one team but not the other.

So to summarize, I'm suggesting
- turn off the ability to push directly to master for those with write access
- turn on requiring PR review/approval for PRs submitted by non-admins
- (optionally) turn on requiring PR review/approval for PRs submitted by admins

Thoughts?

Brian



On Thu, Mar 2, 2017 at 7:00 PM, James Harkins <[hidden email]> wrote:
---- On Thu, 02 Mar 2017 19:01:49 +0800 Rainer Schuetz <[hidden email]> wrote ----

 > I am with the "make contributing easy" and "no enforced distinction between users and developers" side of this discussion. We still need more contribut(ors/ions) and intuitive ways to join the git workflow. It becomes natural after a while, but it is a total show stopper before you've been baptized. One tends to forget that after a while. Some elements in our workflow are still terribly complicated/intransparent (contributing to a submodule and getting the change into master, co-contributing to PRs). Note that using the Webinterface gives you a hint at what forking and branching is, by doing it for you, and telling you about it. Of course non-admins cannot merge anything.
 >
 > I can see what triggered this request into yet more formality that is then ignored. But the actual process is evolutionary, attempts at intelligent design have failed over generations ;) I think the better way to reduce such incidents is to actively promote alternative ways to join discourse around SC (which are also contributions). Being less controlling might need some patience at times, but I believe it will pay in form of a more active community on the long run. The nice side of the emergence of so many fora on SC is that everybody can ultimately get what she is after.

In general, I don't disagree that it's better to keep the entry level for contributing as low as possible without breaking things (and I'll admit to being a bit polemical in my first post). A low bar does assume good faith. Admittedly, it isn't a *huge* problem yet, but we do have a situation arising periodically in which one member Really Really Wants to contribute something -- anything -- but this member also has not taken advice from the community to heart, and persists in submitting PRs of the sort that the community has already said we don't want. I suppose, in principle, tolerance and patience are the best approach. But, when I wake up for each of a few consecutive days and find 80-90 e-mails about this person's last batch of 20 PRs, sure, it tests my patience (partly because the PRs themselves, though well-intentioned, show a lack of respect for developers' time).

About reviews, ideally, they would happen more quickly rather than relaxing standards. You mentioned the case of unforeseen problems with a change. Actually, to me, that's understandable -- I'm a mediocre coder and a worse tester, and I'm perfectly willing to cut slack for not testing everything. That said... the cases that drive me nuts are the ones where something was committed and passed review, but even the most fundamental test case failed -- giving the appearance of a thought process like, "eh, this *ought* to work... push it." I think we can reasonably discourage that approach ;)

hjh



--
_______________________________
Brian Heim
507-429-6468

B.M. '14 University of Texas at Austin
M.M. '16 Yale School of Music
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

amindfv
In reply to this post by Rainer Schuetz

> El 2 mar 2017, a las 07:50, Rainer Schuetz <[hidden email]> escribió:
>

[...]

> In some domains of SC development (e.g. scvim-module, build system, Qt-stuff) this is already too much and puts PRs in an endless waiting queue where it will ultimately stale. At the same time the PR queue grows and contributors get frustrated, and people complain about (often trivial) usability issues in fora not linked to the community controlling the sc code. I've seen people role their eyes when you mention SC ;).

Rainer, can you talk about what these issues are? Are they the known bugs that have just taken a long time to get through the pipeline?

As a side note: getting fixes out to casual users quickly really is much more an issue of frequent small releases and linux distro stuff than it is of commiting to master quickly.

Tom
_______________________________________________
sc-dev mailing list

info (subscription, etc.): http://www.birmingham.ac.uk/facilities/ea-studios/research/supercollider/mailinglist.aspx
archive: http://www.listarc.bham.ac.uk/marchives/sc-dev/
search: http://www.listarc.bham.ac.uk/lists/sc-dev/search/
Reply | Threaded
Open this post in threaded view
|

Re: Github in-web editing: disable?

Rainer Schuetz

On 5 Mar 2017, at 03:33, [hidden email] wrote:

As a side note: getting fixes out to casual users quickly really is much more an issue of frequent small releases and linux distro stuff than it is of commiting to master quickly.

It is both. And in an under-woman-powered community the priority must be getting new contributors and not introducing more formality and control. Frankly, I am getting tired about this talk formal regulations already, and I don't like filling peoples mailboxes with this kind of discussions, which in my view are not about *real* issues.

In general I would say people staying quite means acceptance or non-rejection. But I'll note here that from my side the question of raising control is an exception to that principle. So let's wait what others have to say or leave things as they are. There is enough actual work on SuperCollider to be done...

Best
.r.