-
-
Notifications
You must be signed in to change notification settings - Fork 923
Improved 'Making Good PRs' section to improve clarity and maintainability. #1510
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
a99729e
38a16d6
0cdb96a
5932e90
1c12ebd
980bac0
bd58b77
e43ccdf
d36d02e
057d5b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -183,59 +183,93 @@ message. It is usually okay to leave that as-is and close the editor. | |||||
See `the merge command's documentation <https://git-scm.com/docs/git-merge>`_ | ||||||
for a detailed technical explanation. | ||||||
|
||||||
|
||||||
.. _good-prs: | ||||||
|
||||||
Making good PRs | ||||||
Making Good PRs | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please retain sentence case: https://devguide.python.org/documentation/style-guide/#capitalization
Suggested change
Same applies to the headers below. |
||||||
=============== | ||||||
|
||||||
When creating a pull request for submission, there are several things that you | ||||||
should do to help ensure that your pull request is accepted. | ||||||
Comment on lines
-192
to
-193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the previous introduction was better. |
||||||
|
||||||
#. **Make your change against the right version of Python.** In general all | ||||||
changes are made against the ``main`` branch first. This includes bug fixes. | ||||||
After the change is merged there, it will be :ref:`ported back <branch-merge>` | ||||||
to older :ref:`maintenance releases <branchstatus>` as well. That way we | ||||||
ensure all affected versions are handled. Therefore, basing a new change | ||||||
directly on a maintenance branch is only used in specific circumstances, | ||||||
for instance when that change does not apply to ``main`` or the change | ||||||
requires a different approach in an older Python version compared to | ||||||
``main``. | ||||||
|
||||||
#. **Make sure to follow Python's style guidelines.** For Python code you | ||||||
should follow :PEP:`8`, and for C code you should follow :PEP:`7`. If you have | ||||||
one or two discrepancies those can be fixed by the core developer who merges | ||||||
your pull request. But if you have systematic deviations from the style guides | ||||||
your pull request will be put on hold until you fix the formatting issues. | ||||||
|
||||||
.. note:: | ||||||
Pull requests with only code formatting changes are usually rejected. On | ||||||
the other hand, fixes for typos and grammar errors in documents and | ||||||
docstrings are welcome. | ||||||
Comment on lines
-212
to
-214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove this note and replace it with a shorter one? |
||||||
|
||||||
#. **Be aware of backwards-compatibility considerations.** While the core | ||||||
developer who eventually handles your pull request will make the final call on | ||||||
whether something is acceptable, thinking about backwards-compatibility early | ||||||
will help prevent having your pull request rejected on these grounds. Put | ||||||
yourself in the shoes of someone whose code will be broken by the change(s) | ||||||
introduced by the pull request. It is quite likely that any change made will | ||||||
break someone's code, so you need to have a good reason to make a change as | ||||||
you will be forcing someone to update their code. (This obviously does not | ||||||
apply to new classes or functions; new arguments should be optional and have | ||||||
default values which maintain the existing behavior.) If in doubt, have a look | ||||||
at :PEP:`387` or :ref:`discuss <communication>` the issue with experienced | ||||||
developers. | ||||||
|
||||||
#. **Make sure you have proper tests** to verify your pull request works as | ||||||
expected. Pull requests will not be accepted without the proper tests! | ||||||
|
||||||
#. **Make sure all tests pass.** The entire test suite needs to | ||||||
:ref:`run <runtests>` **without failure** because of your changes. | ||||||
It is not sufficient to only run whichever test seems impacted by your | ||||||
changes, because there might be interferences unknown to you between your | ||||||
changes and some other part of the interpreter. | ||||||
|
||||||
#. Proper :ref:`documentation <documenting>` additions/changes should be included. | ||||||
When creating a pull request, following best practices ensures your contribution is **clear, maintainable, and easy to review**. A well-structured PR improves collaboration and speeds up the review process. | ||||||
|
||||||
1. **Use a Clear and Structured PR Title** | ||||||
|
1. **Use a Clear and Structured PR Title** | |
#. **Use a clear and structured PR title** |
Also please trim trailing spaces, or install pre-commit to do it for you:
https://devguide.python.org/getting-started/setup-building/#install-pre-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 applies to all points not just this one @swastim01
Outdated
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.
See the preview: https://cpython-devguide--1510.org.readthedocs.build/getting-started/pull-request-lifecycle/#making-good-prs
Don't blockquote this. Same for "Avoid".
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.
Accidentally removed line