Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork855
Update the setup and PR lifecycle pages#265
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@ezio-melotti Is this still WIP or ready for review? Lots of good improvements here 👍 |
There are still a few changes I want to make before it's ready -- it shouldn't take too long. |
That's enough commits for this PR :) |
| The automated patch check doesn't actually *answer* all of these | ||
| questions. Aside from the whitespace checks, the tool is | ||
| a memory aid for the various elements that can go into | ||
| making a complete patch. |
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.
This has been moved to the pullrequest page.
| feature/bugfix. | ||
| It is of course okay to pile up several commits to one branch and merge them | ||
| into another in one commit. |
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.
This has been moved to pullrequest and combined with the next red chunk.
| contributed to the resolution, it is good practice to credit them. | ||
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.
This has been combined with the previous chunk.
| (note that not all checks apply to non-core developers). On Windows, use this | ||
| command (after any successful build of Python):: | ||
| python.bat Tools/scripts/patchcheck.py |
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.
This has been combined with the other patchcheck section and has been moved further down.
Thanks@ezio-melotti It's looking great! |
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.
@ezio-melotti One small comment as something to think about. I would merge this PR as is and address that in a future PR if needed. Great work! 🎉 🍰
| 6. Configure an ``upstream`` remote:: | ||
| $ cd cpython | ||
| $ git remote add upstream git@github.com:python/cpython.git |
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.
@ezio-melotti I think this is correct if you have push privileges on the repo, but others likely will need to use https for the upstream repo.
terryjreedySep 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
One has to be using ssh for git@github to work. I believe we agreed to consistently use https: (and git's credential keeper) in the instructions as using both randomly is confusing.
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.
But for the record, it does work even if you don't have push privileges for the repo, if you are using ssh already with github.
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.
This was already discussed on#263. The outcome was that by using this URL we don't need to have separate instructions for core devs and contributors. Pulling works for both, and pushing to upstream would fail anyway if contributors don't have push privileges.
This was also moved from the old instructions, I didn't change it :)
Thanks everyone for the reviews! |
* Update the setup page and the quick guides.* Working and markup tweaks.* Remove the "Tool Setup" section.* Rename the "Creating" section to "Introduction".* Reorganize info about patchcheck.* Rename and move the "Preparation" section to "Making good PRs".* Fix headers levels (by rotating quotes).* Remove outdated comment.* Move and refactor the sections about commits.* Fix markup and long lines.* Convert the step-by-step guide to a bullet list.
This is part of#120, and it builds on#263. The goal of this PR is to reorganize the setup and pullrequest pages.
The end result will be: