-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] DOC: Added rebasing and squashing section to contributing.rst #6176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRG] DOC: Added rebasing and squashing section to contributing.rst #6176
Conversation
@amueller @rvraghav93 could you please take a look at this? Thanks! |
@@ -207,6 +207,51 @@ and Cython optimizations. | |||
<http://astropy.readthedocs.org/en/latest/development/workflow/development_workflow.html>`_ | |||
sections. | |||
|
|||
Rebasing and squashing commits | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct format for header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it gives this kind of a header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. Maybe it would be better to have a heading like this?
I think we can structure it something like this -
|
@rvraghav93 is this a bit better? |
This looks much better. I will take a good look at it in some time |
|
||
2. Make a backup just in case:: | ||
|
||
$ git branch my-feature-branch-bak my-feature-branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? There is reflog in extreme cases no? It would be better if we have it simple I feel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes true....
@rvraghav93 I was thinking of putting the interactive rebasing method before the |
ping @rvraghav93. Sorry for disturbing but should I proceed with the above changes? Could you please have a look at this if you have the time? |
Apologies for the late response! I'll take a look at it now and let you know |
|
||
$ git checkout my-feature-branch | ||
|
||
2. Rebase on master:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should maybe add a line on updating master before?
Thanks a lot for reviewing! I'll address the comments and make the changes asap 👍 |
I've addressed the comments. Hope this is better now :) |
See if you can use your documentation to squash the commits of this PR ;) |
Hahahaha! I'll do that now! |
is this ready for review? If so, please rename. |
I think we should talk first about squashing, then rebasing. That really lowers the chance of having rebase conflicts. |
@amueller oh sorry I'll rename it now. Yes this is ready for review. |
@amueller I have addressed the comments and changed the order of squash and rebase. Could you please check? Thanks! |
^^^^^^^^^^^^^^^^^ | ||
|
||
To keep the history clean and to help backtrack regressions to a single commit we | ||
prefer less (but meaningful) number of commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reword this to we prefer to have less commits that are more meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
umm I'm not quite sure about this.... It still retains the same meaning but makes it unnecessarily verbose. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think it should be we prefer less (but more meaningful) commits
.
|
||
.. note:: | ||
|
||
Detailed guide on solving merge conflicts can be found on `git-scm.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed guide...
-> A detailed guide...
@nelson-liu I've addressed the comments. Good catches! |
@dsquareindia why did you close this? I think it's a great improvement; the part about rebasing is a common question e.g. #6446 (comment) |
@dsquareindia would you mind if i cherry-picked your commit and opened a new pr? I think this addition is quite necessary. |
Sorry for that; I'll open it again. @nelson-liu I'll be happy to continue work on this if needed :) |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
Another way to squash commits is through interactive rebasing which is one the most powerful | ||
features of ``rebase``. If you want to squash the latest 3 commits together (this number can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use $ git log --oneline
to see past commits?
could you also rebase on master? maybe use your own instructions ;) |
@nelson-liu sorry for keeping this PR idle. You can cherry pick this if you want and open a new pr 👍 . Won't be getting a chance to work on this anytime soon... |
I think this needs to be cut back to discuss rebasing etc while working on a PR. We no longer generally want contributors to do squashing, in preference for the "squash and merge" facility provided by github. |
@jnothman I agree and I've seen it happen a few times recently. maybe having this in the docs + perhaps adding it to the PR template would be good tasks for the scipy sprint? |
solved by #7541 IMHO. Feel free to reopen / update. I think rewriting the same docs over and over again is probably not the point. |
Added rebasing and squashing section to
contributing.rst
as agreed here. Used this comment to add to the section.