adding pullrequest guide for #103 #140

louisrawlins · 2017-03-05T20:36:05Z

"The pullrequest page should include a step-by-step list with the required git commands required to create a pull request."

Closes #103

willingc

Thanks @louisrawlins for the PR. I've made some clarifications and suggestions for you. ☀️

willingc · 2017-03-05T20:40:54Z

pullrequest.rst

+
+  git fetch upstream
+
+Use rebase -i to verify what you commit and squish small commits::


/s/squish/squash

willingc · 2017-03-05T20:41:38Z

pullrequest.rst

+`Clear communication`_ is key to contributing to any project, especially an
+`Open Source`_ project like Python.
+
+Here is a quick overview of you can contribute to CPython on Github:


Substitute GitHub for all instances of Github 👍

willingc · 2017-03-05T20:42:14Z

pullrequest.rst

+
+Here is a quick overview of you can contribute to CPython on Github:
+
+1.  `Create an Issue`_ that describes your change, if it doesn't exist and you're explicitly working on something


~~if...something~~

If an issue doesn't already exist, `create one <https://bugs.python.org/>`_?

Clearer. Thanks @ezio-melotti and @willingc

willingc · 2017-03-05T20:43:07Z

pullrequest.rst

+
+5.  `Run tests`_ after you have built Python
+
+6.  `Add an "upstream" Remote in Git`_ on your system 


Suggestion: Add an "upstream" Git remote

willingc · 2017-03-05T20:45:05Z

pullrequest.rst

+
+10. `Push commits`_ to your Github repo
+
+11. `Create Pull Request`_ in Github to merge a branch from your Fork


~~to merge ... Fork~~ -> to ask for your changes to be merged into CPython.

willingc · 2017-03-05T20:46:42Z

pullrequest.rst

+Quick Guide Step-by-step
+''''''''''''''''''''''''
+
+Set up your system::


This will only work if the user has an ssh key setup for GitHub. Otherwise, the user will need to use https.

I'm re-reading this part. How often to contributors not have an SSH / RSA key? To me, that seems standard.

@louisrawlins It's worth mentioning the SSH key since my experience at sprints and hackathons is that many new contributors do not have an SSH key configured for GitHub.

Aha. Gotcha. Hadn't considered hackathons and that environment.

willingc · 2017-03-05T20:47:40Z

pullrequest.rst

+
+Replace **YOUR_GITHUB_ID** with your Github account name above, then add upstream::
+
+  git remote add upstream git://github.com/python/cpython.git


This should probably be the https url since a contributor would need core dev privileges, as well as an ssh key, to use the git url.

Agreed, but I would also add a short note to explain the difference, otherwise users might be wondering why two different types of urls are used.

Yeah. I'm struggling to figure out a way to say that concisely. It's like, you're cloning SSH, then fetching HTTPS -- I suppose?

#130 has a discussion about it that clarifies the differences.

I'm not so much curious about the difference, as I'm trying to figure out how to tell potentially contributors when they might need to worry about the difference.

For me, I treat HTTPS as something I use when I just need to download something as a ZIP, I'm not at my machine, etc. It's not something I'd be using if I'm branching something on my repo and preparing for a Pull Request. For that, I'd use SSH. But perhaps I'm missing a nuance here.

How about "Add an “upstream” Git remote on your system (for SSH, or you can use HTTPS)"?

ezio-melotti

Thanks for the patch! I left a few comments.

ezio-melotti · 2017-03-05T21:38:32Z

pullrequest.rst

+.. _Create a Branch in Git: https://cpython-devguide.readthedocs.io/committing.html#active-branches
+.. _resolve conflicts in Git: https://cpython-devguide.readthedocs.io/committing.html#squashing-commits
+.. _Run tests: https://cpython-devguide.readthedocs.io/runtests.html#running-writing-tests
+.. _Push commits: https://cpython-devguide.readthedocs.io/committing.html#pushing-changes


For all the internal links you should be able to use :ref:`...`.
For short urls (like the one to bpo, or opensource.guide), sometimes it's more readable to add them inline, using `text displayed <url>`_.
For longer urls or urls that are repeated, it's ok using a label like you did.

Can you elaborate on ""For all the internal links you should be able to use :ref:...."" ? I scanned the rST documentation and couldn't quite understand when what got used where

See for example the quick start in index.rst -- it links to other sections using this syntax.
See also https://cpython-devguide.readthedocs.io/documenting.html#cross-linking-markup for more info.

I'm not sure I follow. This appears to be internal linking within a page whereas I'm linking within the same site, but to different pages. Perhaps I don't understand the full scope of what :ref: is doing for devguide.

Okay. I think I've got it... but where can I find an index of each of these unique IDs across devguide? Is there one, or can Sphinx create it for me? I want to avoid name collision.

Plus, I'm finding it difficult to do "ACTIVE VERB" -- GET, DO, RUN, versus the current titles which are like Getting, Doing, Running. They work fine as headers, but are tough for step-by-step instructions that say, "Do step 1."; "Run step 2" and so on

Sorry for continued complaints, as this might be a known issue. I've updated the devguide with internal :ref: links, but what it's gained in engineering clarity I think has degraded the readability of steps and hyperlinks -- I'm open to thoughts on that.

Note that when the auto-generated text for a link target doesn't make sense in the context of the particular link, you can instead use:

:ref:`link text <link-target>`

The description at https://cpython-devguide.readthedocs.io/documenting.html#cross-linking-markup mentions using that syntax for linking to labels that don't refer to section headings, but doesn't point out that it can also be used to link to section headings with text other than the section heading name.

As far as name collisions are concerned, I don't know of an easy way to get a complete listing, but Sphinx itself will complain about any duplicates (and tell you which files contain the conflicting labels) as part of the build process.

Thank you, @ncoghlan That is super-helpful.

ezio-melotti · 2017-03-05T21:40:09Z

pullrequest.rst

+
+Here is a quick overview of you can contribute to CPython on Github:
+
+1.  `Create an Issue`_ that describes your change, if it doesn't exist and you're explicitly working on something


If an issue doesn't already exist, `create one <https://bugs.python.org/>`_?

ezio-melotti · 2017-03-05T21:43:13Z

pullrequest.rst

+
+8.  Commit changes and `resolve conflicts in Git`_
+
+9.  `Run tests`_ again


I think this should be between 7 and 8: after creating the branch, the user should fix the issue and run the tests to make sure the fix works. If it works, the changes can be committed.

Good catch. I meant to do just that

ezio-melotti · 2017-03-05T21:44:24Z

pullrequest.rst

+
+11. `Create Pull Request`_ in Github to merge a branch from your Fork
+
+12. Celebrate! You just contributed to Python. :)


Perhaps before celebrating the user should address the review comments and update the PR until it gets merged.

You're telling me! Haha

ezio-melotti · 2017-03-05T21:45:41Z

pullrequest.rst

+
+Replace **YOUR_GITHUB_ID** with your Github account name above, then add upstream::
+
+  git remote add upstream git://github.com/python/cpython.git


Agreed, but I would also add a short note to explain the difference, otherwise users might be wondering why two different types of urls are used.

ezio-melotti · 2017-03-05T21:46:56Z

pullrequest.rst

+
+  git checkout -b MY_NEW_FEATURE upstream/master
+
+As you work, please create a separate commit for each bug fix or feature change::


This might be a bit misleading, since each PR should either fix a bug or add a new feature.

ezio-melotti · 2017-03-05T21:48:55Z

pullrequest.rst

+
+Use rebase -i to verify what you commit and squish small commits::
+
+  git rebase -i upstream/master


Is this step necessary?
IIUC as long as there are not conflicts, both the rebase and the squash are not necessary, since the committer will do it.
If squashing prevents reviewers to see the individual commits, it should also be avoided by contributors.

What do you think? I'm not sure. This came from a workflow where many devs were contributing to a single remote branch, then the dev lead would merge that branch to production periodically. Since I'm not contributing to the CPython codebase day-to-day, it's hard for me to judge

The squash seems unnecessary, especially if it "hides" the intermediate commits.
The rebase shouldn't hurt, but I think it would be necessary only in case of conflicts, otherwise it's just an extra step that might be avoided.
Someone more familiar with Git than me could comment.

git rebase was always useful if I was committing to a file that I knew was likely to be touched by other devs. It's a relatively explicit "walkthrough" process that is built into Git. Pt another way, if you're familiar enough to programming to contribute to CPython, it's likely you can figure out rebase. I can go either way on squashing commits -- it makes things clear in the case content and cosmetic changes, but for logic I don't generally find it helpful

I tend to lean toward @ezio-melotti's approach to not rebase since the core dev will take care of it. @louisrawlins, In my experience contributors vary widely in their understanding of rebase, so let's keep things easiest and not recommend. Thanks.

ezio-melotti · 2017-03-05T21:50:19Z

pullrequest.rst

+
+Work on new features or fixes::
+
+  git checkout -b MY_NEW_FEATURE upstream/master


I would use something more generic, such as MY_BRANCH_NAME, or issue-NNNNN

ezio-melotti · 2017-03-05T21:51:34Z

pullrequest.rst

+
+When you're ready, make a Pull Request on Github and refer to your branch named **MY_NEW_FEATURE**.
+
+*Though submitting a Pull Request on Github is the preferred method of contribution, you can alternatively upload a patch to bugs.python.org*


This line (and perhaps the previous one) is too long. Add http:// in front of b.p.o so it becomes a clickable link.

Having emphasis * and the link on the same line caused issues. I've made it a "Note:"

ezio-melotti · 2017-03-05T21:53:09Z

pullrequest.rst

+
+  git push origin MY_NEW_FEATURE
+
+When you're ready, make a Pull Request on Github and refer to your branch named **MY_NEW_FEATURE**.


The contributors should be ready as soon as they pushed their branch :)
I'm also not sure what you mean with "refer to your branch named ..." -- isn't it enough to click on the "create PR" button?

As far as I know, there is that extra step where I have to go to [CPython on GitHub] (https://github.com/python/cpython), click "New pull request", find and click on the link "compare across forks", select my repository, THEN select my branch -- I find the entire process burdensome (because I'm doing everything else on the CLI) and a bit confusing because it's its own process -- how much detail should we have in the docs?

What I did so far was:

push the new branch to my fork

go to my fork on github

a button saying something like "you pushed branch X [create PR]" appears

click on the button

IOW, I create the PR from my fork, not from the upstream CPython.

If you go to your fork, the GitHub UI should pop up the option to submit a PR as @ezio-melotti recommends.

Okay. Clearer. It's the "New pull request" button next to the "branch selection" menu in my repo, correct? Glad I asked! (To be clear, I wasn't creating the PR from the upstream, but rather going TO CPython because I had a Pull Request FOR CPython -- it didn't make sense to fiddle with a PR from my own repo.)

louisrawlins · 2017-03-11T20:11:42Z

Hi @ezio-melotti . Thanks for all the feedback. Take a look and let me know if there's anything else that needs updating. @willingc , thank you too for your help.

willingc

Add the SSH key reference and this looks good to me.

louisrawlins · 2017-03-11T21:18:31Z

Update all .git references to _use HTTPS: https://help.github.com/articles/which-remote-url-should-i-use/

willingc

Thanks @louisrawlins. This looks ready to merge. ☀️

Mariatta · 2017-03-11T23:36:33Z

pullrequest.rst

+
+6.  Set your Git :ref:`remote-configuration` to add an "upstream" remote (using SSH, or you can `use HTTPS`_)
+
+7.  Create :ref:`committing-active-branches` in Git where you can work on changes


I think the reference to committing-active-branches is incorrect here.
Currently it links to the section that explains how to list the branches (git branch command.)
It's better to refer to line 153 below (about git checkout -b BRANCH_NAME) which is the key step.

Thanks @Mariatta . I agree. That feels clearer. I was struggling to move back-and-forth between comitting.rst -- some of the naming and explanation didn't fit.

louisrawlins · 2017-03-23T23:54:33Z

Thank for your patience. Let me know, @willingc, @Mariatta and @ezio-melotti if this hits the mark. @DimitrisJim -- I'm hoping everything is resolved for the CLA. Just holler if something should be added to the quick guide.

DimitrisJim · 2017-03-24T17:13:29Z

pullrequest.rst

@@ -105,23 +105,23 @@ Here is a quick overview of you can contribute to CPython on GitHub:

 1.  If an Issue doesn't exist, `create an Issue`_ that describes your change

-2.  Prepare your system by :ref:`setup`
+2.  :ref:`Get started <setup>`_ and set up your system


Cross references don't use _ prefix, so you should remove these; this is why travis failed on you. In general, run make html on your changes to catch all warnings :-). Apart from that, LGTM :-)

Erp! Thank you! I thought I might have messed up some of the references, but got caught up in rST particulars. Thanks -- will check that more-closely next time. For better of worse, Sphinx didn't throw any errors for me locally.

Ah yeah. 😅

The reason I didn't see any errors, is that I failed to check in & push my final change. Thanks again!

willingc · 2017-04-12T20:31:19Z

@Mariatta @ncoghlan Do you mind if we merge this as it is and iterate later if needed? @louisrawlins did a great job with this PR and addressing feedback and I don't want it to languish ☀️

Thanks for your patience @louisrawlins. 🍪

Mariatta · 2017-04-12T21:58:22Z

Thanks @louisrawlins and congrats for your first contribution to the Dev Guide! 🎉

willingc · 2017-04-12T22:55:02Z

Congrats @louisrawlins 🍰

Closes python#103

adding pullrequest guide for python#103

9e3cc06

the-knights-who-say-ni added the CLA signed label Mar 5, 2017

willingc reviewed Mar 5, 2017

View reviewed changes

ezio-melotti requested changes Mar 5, 2017

View reviewed changes

ezio-melotti self-assigned this Mar 5, 2017

DimitrisJim mentioned this pull request Mar 7, 2017

Explain how to fill out the CLA #142

Closed

louisrawlins added 14 commits March 11, 2017 10:48

replaces GitHub, word caps

b62d78d

edits - create issue

21b4960

edits - upstream remote

0b68c42

clearer create issue

523b77f

edits - cpython pull req

e0aac2a

edit - pull req

24149d3

edit - run tests, then commit

044dc08

https or ssh

ca8e9c1

clarifying when to celebrate a pr

a3ddf1c

adding refs

a6b773c

using internal refs when available"

ab07aef

swaping feature for branch

cebd349

clarifying squishing and prs

2b468f7

alt, submit patch to bpo

36eb29b

willingc requested changes Mar 11, 2017

View reviewed changes

added ssh https ref

1ae31e5

willingc approved these changes Mar 11, 2017

View reviewed changes

Mariatta reviewed Mar 11, 2017

View reviewed changes

review adds to guide

9865ccf

DimitrisJim reviewed Mar 24, 2017

View reviewed changes

proper rst refs

2a7bb10

DimitrisJim approved these changes Mar 24, 2017

View reviewed changes

ezio-melotti approved these changes Mar 24, 2017

View reviewed changes

ezio-melotti removed their assignment Mar 24, 2017

Mariatta approved these changes Apr 12, 2017

View reviewed changes

Add a missing word

a37d2e6

Mariatta merged commit f899455 into python:master Apr 12, 2017

This was referenced Apr 13, 2017

#133: Update Devguide pull request commit documentation #134

Closed

Update Devguide about pull request commit documentation. #133

Closed

willingc mentioned this pull request Mar 19, 2018

Preferred cloning URL #130

Closed

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022

Add Pull Request Step-By-Step Guide (python#140)

3841ed3

Closes python#103


		git fetch upstream

		Use rebase -i to verify what you commit and squish small commits::


		Here is a quick overview of you can contribute to CPython on Github:

		1. `Create an Issue`_ that describes your change, if it doesn't exist and you're explicitly working on something


		5. `Run tests`_ after you have built Python

		6. `Add an "upstream" Remote in Git`_ on your system


		10. `Push commits`_ to your Github repo

		11. `Create Pull Request`_ in Github to merge a branch from your Fork


		Replace YOUR_GITHUB_ID with your Github account name above, then add upstream::

		git remote add upstream git://github.com/python/cpython.git


		8. Commit changes and `resolve conflicts in Git`_

		9. `Run tests`_ again


		11. `Create Pull Request`_ in Github to merge a branch from your Fork

		12. Celebrate! You just contributed to Python. :)


		git checkout -b MY_NEW_FEATURE upstream/master

		As you work, please create a separate commit for each bug fix or feature change::


		Use rebase -i to verify what you commit and squish small commits::

		git rebase -i upstream/master


		Work on new features or fixes::

		git checkout -b MY_NEW_FEATURE upstream/master


		When you're ready, make a Pull Request on Github and refer to your branch named MY_NEW_FEATURE.

		Though submitting a Pull Request on Github is the preferred method of contribution, you can alternatively upload a patch to bugs.python.org


		git push origin MY_NEW_FEATURE

		When you're ready, make a Pull Request on Github and refer to your branch named MY_NEW_FEATURE.


		6. Set your Git :ref:`remote-configuration` to add an "upstream" remote (using SSH, or you can `use HTTPS`_)

		7. Create :ref:`committing-active-branches` in Git where you can work on changes

adding pullrequest guide for #103 #140

adding pullrequest guide for #103 #140

Conversation

louisrawlins commented Mar 5, 2017 • edited by Mariatta Loading

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezio-melotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisrawlins Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisrawlins commented Mar 11, 2017

willingc left a comment

Choose a reason for hiding this comment

louisrawlins commented Mar 11, 2017

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisrawlins commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisrawlins Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

willingc commented Apr 12, 2017

Mariatta commented Apr 12, 2017

willingc commented Apr 12, 2017

louisrawlins commented Mar 5, 2017 •

edited by Mariatta

Loading

louisrawlins Mar 5, 2017 •

edited

Loading

louisrawlins Mar 24, 2017 •

edited

Loading