DOC better clarify upstream synchronization in contributing guide by adrinjalali · Pull Request #13505 · scikit-learn/scikit-learn

adrinjalali · 2019-03-24T10:41:25Z

Changes the contributing guides to be explicit about how upstream/master synchronization can be done.

jnothman · 2019-03-24T11:17:43Z

doc/developers/contributing.rst

+  $ git merge upstream/master

-with ``master`` being synchronized with the ``upstream``.
+In order to synchronize your ``master`` with the ``upstream``'s (assuming you


I don't understand this. I don't understand the benefit of teaching rebase either.

Maybe git pull upstream master would be enough?

I'd remove the "(assuming you usually work on branches other than your local's master)" part, since we shouldn't give users the idea that it could be done another way.

The rebase is just to leave users with a clean master history on their local copy, which I think is good practice and useful later on when users do git log stuff.

The "assuming..." part is kinda to tell users that's how a good practice may look like. Not all users create branches and they may just work on their master. My idea here is to give users an idea of how they can be doing the git-workflow by giving them hints and keywords.

Questions such as #13496 (comment) are not rare among new contributors, and there's a gap between "here's how you add an origin" and "assuming your master is synced with master" in our documentation.

@jnothman I'm not sure which part is not clear here to you. I'm happy to expand and clarify this part, I was trying to keep the additions minimal.

I see. Then what about something like this

It is recommended to work on temporary branches rather than directly on master. Since these temporary branches are branched off master, it is helpful to keep your local master up to date with the upstream's master. Do do this, you can...

I like that, change the wording.

…into doc/git-rebase

NicolasHug

Still not convinced by rebase over pull but I don't mind, LGTM

doc/developers/contributing.rst

jnothman · 2019-03-25T02:22:50Z

doc/developers/contributing.rst

+``master``. To do this, you can::
+
+  $ git fetch upstream
+  $ git rebase upstream/master


well, I would usually do git reset --hard upstream/master here.

+1 although it's not necessary to ever use update the local master branch if all the feature branches are branched from upstream/master as recommended in the previous comment.

well, I would usually do git reset --hard upstream/master here.

reset --hard is fine as long as the user really knows they're not in their feature branch. I rather not have a command here which could be copy pasted by accident and result in the user loosing their changes.

jnothman · 2019-03-25T02:23:36Z

doc/developers/contributing.rst

-with ``master`` being synchronized with the ``upstream``.
+It is recommended to work on temporary branches rather than directly on
+``master``. Since these temporary branches are branched off ``master``, it is
+helpful to keep your local ``master`` up to date with the upstream's


Maybe rather than advising this, we should just recommend creating feature branches with:

git fetch upstream get checkout -b myfeaturename upstream/master

ogrisel

+1 once @jnothman's suggestions are taken into account.

ogrisel · 2019-03-25T09:55:49Z

doc/developers/contributing.rst

+``master``. To do this, you can::
+
+  $ git fetch upstream
+  $ git rebase upstream/master


+1 although it's not necessary to ever use update the local master branch if all the feature branches are branched from upstream/master as recommended in the previous comment.

adrinjalali · 2019-03-25T10:27:08Z

Is this now a good compromise? No rebase mentioned! (I'm genuinely curious why we don't want to mention it though)

ogrisel · 2019-03-25T10:55:23Z

LGTM.

qinhanmin2014 · 2019-03-25T13:35:10Z

doc/developers/contributing.rst

+  $ git merge upstream/master

-with ``master`` being synchronized with the ``upstream``.
+It is recommended to work on temporary branches rather than directly on


We're talking about how to solve the conflict when merging master from upstream. I can't understand why we mention these things here. (Notice that the next paragraph starts with "Subsequently")

Fair point. Should we move it right after where the user clones the fork, and
add the upstream remote etc etc?

Just saw that @qinhanmin2014's review is similar to mine :)

We already have

Create a branch to hold your development changes: $ git checkout -b my-feature

Anyway, this is not a big problem, as long as you can put these things in a suitable place, I won't oppose.

albertcthomas

Thanks @adrinjalali. We will be able to point the contributors to this section if they ask how to do this.

albertcthomas · 2019-03-25T13:45:00Z

doc/developers/contributing.rst

+  $ git merge upstream/master

-with ``master`` being synchronized with the ``upstream``.
+It is recommended to work on temporary branches rather than directly on


It can be a bit confusing for a first time contributor to read this if he already did point 5. hereabove. We should maybe say : "As explained in 5, it is recommended to work [....]. You can also create your feature branch from the latest upstream/master:"

And I would put this paragraph after the paragraph about solving conflicts so that the one about solving conflicts is just after the one explaining how to merge master.

adrinjalali · 2019-03-25T16:50:18Z

This should now address the comments.

albertcthomas

Otherwise LGTM

albertcthomas · 2019-03-25T17:20:21Z

doc/developers/contributing.rst

+  of it you can::
+
+        $ git fetch upstream
+        $ git checkout -b mybranchname upstream/master


I would reuse the same name as above for the branch my-feature.

jnothman · 2019-03-25T23:12:36Z

doc/developers/contributing.rst

+  of it you can::
+
+        $ git fetch upstream
+        $ git checkout -b my-feature upstream/master


Maybe it's worth just changing the instructions above rather than adding a note. But I'm fine with this

adrinjalali · 2019-03-26T09:43:19Z

I suppose it's hard to make everybody very happy here, but seems we have enough +1s, so merging.

…ikit-learn#13505) * clarify upstream synchronization * apply Nicolas's comment * fix typo * don't mention the rebase * move the notes * fix branch name

…uide (scikit-learn#13505)" This reverts commit bb8d093.

…ikit-learn#13505) * clarify upstream synchronization * apply Nicolas's comment * fix typo * don't mention the rebase * move the notes * fix branch name

clarify upstream synchronization

e5e6a5e

adrinjalali changed the title ~~clarify upstream synchronization~~ DOC better clarify upstream synchronization in contributing guide Mar 24, 2019

jnothman reviewed Mar 24, 2019

View reviewed changes

adrinjalali added 2 commits March 24, 2019 17:43

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

8499d51

…into doc/git-rebase

apply Nicolas's comment

c9900f5

NicolasHug approved these changes Mar 24, 2019

View reviewed changes

doc/developers/contributing.rst Outdated Show resolved Hide resolved

fix typo

39ed60f

jnothman reviewed Mar 25, 2019

View reviewed changes

ogrisel approved these changes Mar 25, 2019

View reviewed changes

don't mention the rebase

da7ccda

qinhanmin2014 reviewed Mar 25, 2019

View reviewed changes

albertcthomas reviewed Mar 25, 2019

View reviewed changes

move the notes

8edc3e8

albertcthomas approved these changes Mar 25, 2019

View reviewed changes

fix branch name

1185b14

jnothman approved these changes Mar 25, 2019

View reviewed changes

adrinjalali merged commit ab399a6 into scikit-learn:master Mar 26, 2019

adrinjalali deleted the doc/git-rebase branch March 26, 2019 09:43

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "DOC better clarify upstream synchronization in contributing g…

d1aaaea

…uide (scikit-learn#13505)" This reverts commit bb8d093.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "DOC better clarify upstream synchronization in contributing g…

a234590

…uide (scikit-learn#13505)" This reverts commit bb8d093.

Uh oh!

Conversation

adrinjalali commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Mar 25, 2019

Uh oh!

ogrisel commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertcthomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Mar 25, 2019

Uh oh!

albertcthomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adrinjalali commented Mar 24, 2019 •

edited

Loading