openopensource / openopensource.github.io

https://openopensource.github.io
168 stars 21 forks source link

Unclear what "internal pull request" is #10

Open benjaoming opened 7 years ago

benjaoming commented 7 years ago

Internal PRs are mentioned twice in the list:

  1. External API changes and significant modifications ought to be subject to an internal pull-request to solicit feedback from other contributors.
  2. Internal pull-requests to solicit feedback are encouraged for any other non-trivial contribution but left to the discretion of the contributor.

But IMO it's unclear what exactly they are. Also talking about "external API" at the same time makes it slightly more fuzzy language-wise.

Is there perhaps a better term for this?

strugee commented 7 years ago

I don't think there's a better term (at least not that I can think of). But perhaps we could define it with a footnote or something?

strugee commented 7 years ago

What an "internal Pull Request" is, by the way, is where you open a Pull Request even though you have the rights to merge the relevant branches. This allows other project contributors to review your code in the same way that an "external" contribution would be reviewed, which is a Good Thing™ because it increases overall code quality.

benjaoming commented 7 years ago

Ah okay, I understand what you mean.

"All changes should go through Pull Requests - even owners of the repository should open Pull Requests for all their changes"

I think one consequence is that owners of a repository should always work in their own private fork in order to avoid leaving branches around in the original main repository..

strugee commented 7 years ago

Why is it bad to leave branches around?

benjaoming commented 7 years ago

If those branches are visible to others, they will clutter their view. Features and bug fixes happen through branches that aren't relevant after they've been merged - often the branches are forgotten and left dangling in remote repos. That's why it's better to store feature and bug fix branches in your own repo than in a shared organization's repo.

strugee commented 7 years ago

@benjaoming that feels like premature optimization to me, for a couple reasons:

  1. GitHub branch views differentiate between "active" and "stale" branches
  2. git branch only shows local branches, not branches fetched from remotes, so it'll only show other peoples' branches that you've explicitly checked out locally (in which case it seems useful for those to be displayed anyway)
  3. Pull Requests prompt you to delete branches after merge, so that helps clean things up too
benjaoming commented 7 years ago

@strugee I think I've sidetracked the original decision and whether you agree that it's easier to write what we mean than to introduce and define an unknown term?

As for the other stuff, it's a Github/Git UI thing and you're right that organizations can have branches in their main repos. I just see a lot of orgs getting it wrong, myself included :) But I don't think it's possible to categorically rule out the possibility that a lot of people and maintainers get it right!

screenshot from 2017-05-02 19-38-17

cben commented 7 years ago

Do all "open open source" projects even have a github org? Because if you just work in $you/$repo, it will include your feature branches (at least for your active PRs) by definition. (which is one of the reasons creating an org up front is not a bad idea. but I don't see that mentioned anywhere. is there any separate "how to make your project open open source" guidance?)

benjaoming commented 7 years ago

@cben maybe it's better to address it separately, since perhaps it's clouding the other decision :)

strugee commented 7 years ago

I think I've sidetracked the original decision and whether you agree that it's easier to write what we mean than to introduce and define an unknown term?

Ah, sorry.

In general I think it's easier to introduce and define the term, since while this isn't a super common term (like just plain "Pull Request" is), it's not something this project made up either. That being said I don't really feel strongly about this.

I also forgot about the branch view you posted a screenshot of - nice catch :)

Forked that discussion to #10.