Skip to content
This repository has been archived by the owner on Jan 25, 2019. It is now read-only.

Fix Github-101 documentation page following GitHub redesign #58

Closed
tobie opened this issue Jul 16, 2013 · 7 comments
Closed

Fix Github-101 documentation page following GitHub redesign #58

tobie opened this issue Jul 16, 2013 · 7 comments
Assignees
Labels

Comments

@tobie
Copy link
Member

tobie commented Jul 16, 2013

/cc @r12a

@ghost ghost assigned tobie Jul 16, 2013
@lmclister
Copy link
Member

I can take this one.... I need it for Shanghai anyway :/

@r12a
Copy link

r12a commented Jul 22, 2013

It might be good, if you have time, to add just a few words about what
happens after the tests have been reviewed and approved.

I'm guessing that you need to check the web interface for the words:

[[
Pull request successfully merged and closed

You're all set—the r12a:r12a/html5-character-encoding branch can be
safely deleted.
]]

I'm assuming that that means
[1] the tests are installed on the server and need no further attention,
and
[2] that at that point you should delete the branch, rather than leave
it open, and
[3] that the submitter doesn't need to take any further action, apart
from deleting the branch.

It would be nice (if i'm right) to have a paragraph that confirmed those
assumptions.

Cheers,
RI

On 19/07/2013 21:56, Larry McLister wrote:

I can take this one.... I need it for Shanghai anyway :/


Reply to this email directly or view it on GitHub
#58 (comment).

Richard Ishida, W3C
http://rishida.net/

@lmclister
Copy link
Member

re: deleting the branch after a PR is accepted, I think it probably depends upon the workflow.

I'd like to get this into the git doc but want to make sure we agree, so take a look at the cases below and lmk what you think. Also, what cases are missing?

Say I'm writing flexbox tests,... I create the branch lmclister/flexboxtests. I complete some tests and submit a PR.

Case 1 - Nobody has looked at my PR but I want to create more flexbox tests. This is just the normal update PR workflow. I reuse the branch / update the PR.

Case 2 - Somebody has provided me feedback on my PR. Now I really should go address that feedback on the PR but I really just want to write a few more tests. In this case, make a new branch, agreed? if so, any preferences on branch naming?

Case 3 - Somebody has accepted my PR. I reuse the branch for my next set of flexbox tests.

Case 4 - I want to create some mulitcol tests (or some other feature) and there are no outstanding PRs on lmclister/flexboxtests. Nevertheless, I shouldn't reuse lmclister/flexboxtests, instead I should create a lmclister/multicoltests branch. If the branch name was more general it wouldn't matter, so I suppose it is a matter of if we care about branch names.

@tobie
Copy link
Member Author

tobie commented Jul 30, 2013

re: deleting the branch after a PR is accepted, I think it probably depends upon the workflow.

We take the risk of bumping into hard merge problems when we tell people to reuse branches. 90% of my time helping folks with Git is to fix pull requests which branching issues. We make it a lot easier on ourselves if we have one unique consistent story to tell: start from master every time, make small topic branches, name them precisely, delete them once they've been pulled.

Say I'm writing flexbox tests,... I create the branch lmclister/flexboxtests. I complete some tests and submit a PR.

You want to choose a more precise name for reasons you outline later yourself.

Case 1 - Nobody has looked at my PR but I want to create more flexbox tests. This is just the normal update PR workflow. I reuse the branch / update the PR.

In general, I would advise against this. The smaller the PR the faster it'll get reviewed and the bigger the sense of accomplishment for everyone involved.

Case 2 - Somebody has provided me feedback on my PR. Now I really should go address that feedback on the PR but I really just want to write a few more tests. In this case, make a new branch, agreed?

Absolutely.

if so, any preferences on branch naming?

Pick a name that describes precisely what it is you're working on. Note that you might want to do something like: lmclister/flexbox/flex-grow-prop.

Case 3 - Somebody has accepted my PR. I reuse the branch for my next set of flexbox tests.

No, you really want to start a new branch from master. Branches are cheap in Git. More precise naming would allow you to avoid this temptation.

Case 4 - I want to create some mulitcol tests (or some other feature) and there are no outstanding PRs on lmclister/flexboxtests. Nevertheless, I shouldn't reuse lmclister/flexboxtests, instead I should create a lmclister/multicoltests branch. If the branch name was more general it wouldn't matter, so I suppose it is a matter of if we care about branch names.

We care about branch names to avoid name collisions and to scope the tests to something palatable. That's pretty much all there is to it.

In general, the Git flow is about small commits, and small topic branches. This makes it a much easier to get early feedback, maintain momentum, etc.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 30, 2013

Very much yes about small branches/patches. I don't want to review a PR to fix all outstanding issues with all XHR tests (like web-platform-tests/wpt#103), though I'd probably have been happy to review any of those commits on their own. (The workflow I use is:

git checkout master
git checkout -b my-new-subject
...
git add ...
git commit -m "..."
git push
git checkout master

.)

@lmclister
Copy link
Member

Great, thank you for the feedback.

@tobie
Copy link
Member Author

tobie commented Aug 12, 2013

Sounds like #65 closes this one.

@tobie tobie closed this as completed Aug 12, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants