adding pullrequest guide for #103

[louisrawlins]

"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]

/s/squish/squash

[willingc]

Substitute GitHub for all instances of Github 👍

[willingc]

if...something

[ezio-melotti]

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

[louisrawlins]

Clearer. Thanks @ezio-melotti and @willingc

[willingc]

Suggestion: Add an "upstream" Git remote

[willingc]

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

[willingc]

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

[louisrawlins]

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

[willingc]

@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.

[louisrawlins]

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

[willingc]

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.

[ezio-melotti]

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.

[louisrawlins]

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?

[ezio-melotti]

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

[louisrawlins]

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.

[louisrawlins]

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]

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.

[louisrawlins]

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

[ezio-melotti]

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.

[louisrawlins]

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.

[louisrawlins]

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.

[louisrawlins]

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

[louisrawlins]

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.

[ncoghlan]

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.

[ncoghlan]

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.

[louisrawlins]

Thank you, @ncoghlan That is super-helpful.

[ezio-melotti]

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

[ezio-melotti]

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.

[louisrawlins]

Good catch. I meant to do just that

[ezio-melotti]

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

[louisrawlins]

You're telling me! Haha

[ezio-melotti]

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]

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

[ezio-melotti]

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.

[louisrawlins]

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

[ezio-melotti]

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.

[louisrawlins]

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

[willingc]

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]

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

[ezio-melotti]

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

[louisrawlins]

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

[ezio-melotti]

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?

[louisrawlins]

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?

[ezio-melotti]

What I did so far was:

1. push the new branch to my fork
2. go to my fork on github
3. a button saying something like "you pushed branch X [create PR]" appears
4. click on the button

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

[willingc]

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

[louisrawlins]

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]

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]

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]

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.

[louisrawlins]

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]

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]

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 :-)

[louisrawlins]

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.

[louisrawlins]

Ah yeah. 😅

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

[willingc]

@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]

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

[willingc]

Congrats @louisrawlins 🍰