Repository navigation
Issue #520: Made https://devguide.python.org/committing/ simpler - #650
Conversation
| * **Do the changes meet the requirements of the patch checklist?** | ||
| :ref:`Run patchcheck <patchcheck>` to perform a quick confidence | ||
| check on the changes. |
There was a problem hiding this comment.
Should core developers run patchcheck? Doesn't CI run patchcheck?
There was a problem hiding this comment.
@methane Thanks for your feedback. I included this item in the checklist because the current version of this page includes the following section:
Patch checklist
'''''''''''''''
You should also :ref:`run patchcheck <patchcheck>` to perform a quick
sanity check on the changes.
If core developers don't need to run patchcheck, then I have no objection to removing the checklist item.
There was a problem hiding this comment.
@methane I removed this item from the checklist.
There was a problem hiding this comment.
I consider this a mistake. See review.
|
@brettcannon I'd be interested to know if the changes that I'm proposing with this PR are what you were going for when you said that the page on accepting pull requests needed to be simplified. |
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
Please revert the removal of the patchcheck advice and edit it instead.
Patchcheck is mainly checking for extra whitespace at the end of lines and files. The devguidev should say that. If CI patchcheck fails, CI fails. So people should run patchcheck or the equivalent before making a PR, just as they should run unittests before doing so, and not leave it to CI to catch test failures even on their development OS.
|
@terryjreedy Thanks for your feedback. I put that checklist item back in and modified it slightly. |
I agree that devguide should note about run patchcheck. But I disagree about adding it in "Assessing a pull request" section. Core developers don't have to run patchcheck manually before merging the PR if CI status is green, do it? |
|
No. Running patchcheck locally, like running unittests locally, is something to do before making a PR, so that the future PR is likely to pass CI tests at least on the same OS the patch was developed on. It is frustrating when an otherwise good PR is blocked waiting for a trailing whitespace fix. |
"Assessing a pull request" is the section for "reviewing" PRs. Not for "creating" PRs. What should be done when making PR is documented in https://devguide.python.org/pullrequest/ already. |
brettcannon
left a comment
brettcannon
left a comment
There was a problem hiding this comment.
Looks good overall, but there's some simplifications that I think we can make.
| * **Was the pull request first made against the** ``master`` **branch?** | ||
| The only branch that receives new features is ``master``, the | ||
| in-development branch. |
There was a problem hiding this comment.
PRs can be made against bugfix branches if the issue is only in that version.
| * **Was the pull request first made against the** ``master`` **branch?** | |
| The only branch that receives new features is ``master``, the | |
| in-development branch. | |
| * **Was the pull request first made against the appropriate **branch?** | |
| E.g. the only branch that receives new features is ``master``, the | |
| in-development branch. Pull requests should only target bug-fix branches | |
| if an issue only appears in that (and potentially older) versions. |
There was a problem hiding this comment.
@brettcannon Thanks for your feedback! I tweaked the formatting and wording in your suggestion and then committed my own version. Please let me know what you think.
| * **Do the changes meet the requirements of the patch checklist?** | ||
| :ref:`Run patchcheck <patchcheck>` to perform a confidence check and | ||
| flag extra whitespace. |
There was a problem hiding this comment.
Core devs won't be running make patchcheck manually since the code is up on GitHub and that check should be covered by a status check. Perhaps the "test suite passes", this step, and any other steps that are implicitly covered by PR status checks can be merged into a single, "Make sure the appropriate status checks are passing"?
There was a problem hiding this comment.
@brettcannon There was already a checklist item for the test suite, so I combined the two checklist items and removed the specific mention of patchcheck.
| * **Were** ``pyconfig.h.in`` **and** ``configure`` **regenerated?** | ||
| Regenerate them if necessary. |
There was a problem hiding this comment.
There are actually a litany of files that need to be regenerated (e.g. zipimport and import-related files as I know you're familiar with 😄 ). This should be covered by a status check, so no need to call it out.
There was a problem hiding this comment.
@brettcannon I removed this item. Happy to hear that you're listening to the Real Python podcast 😄
There was a problem hiding this comment.
I actually didn't get it from the podcast, just from reviewing your zipimport PR. :)
| * **Was the contributor added to** ``Misc/ACKS`` **?** | ||
| Make sure that the patch is attributed correctly with the contributor's | ||
| name in ``Misc/ACKS``. If the patch has been heavily modified, then | ||
| "Initial patch by <x>" is an appropriate alternate wording. GitHub now | ||
| supports `multiple authors | ||
| <https://help.github.com/articles/creating-a-commit-with-multiple-authors/>`_ | ||
| in a commit. Add ``Co-authored-by: name <name@example.com>`` at the end | ||
| of the commit message. |
There was a problem hiding this comment.
Since git stores this information in the actual commit metadata, I don't think this step is really necessary anymore.
There was a problem hiding this comment.
@brettcannon I removed this item too. Thanks for all your feedback. I'm looking forward to getting this out!
If there are any other parts of the documentation that you specifically wish someone was working on, then please feel free to let me know. I'm looking forward to the documentation work group getting started, so I'm picking away at issues in the meantime.
Co-authored-by: Brett Cannon <brett@python.org>
|
@brettcannon Are there any other changes that you'd like me to make? |
|
@jablonskidev only if it involves spacetime to give me more so I can find the team to do another review. 😉 This is at the top of my PR review list. |
|
Thanks, @jablonskidev ! |
5 participants
These changes are based on issue #520.
Issue #520 was somewhat open ended, so I generally tried to make the text easier to skim so that core developers could use it as a quick reference.
I'm still learning reStructuredText, so there are some issues with formatting, some of which have caused problems with line lengths in vertical lists.
I'd be interested to know if this kind of simplification is what you were going for. I'm looking forward to hearing how we can keep improving this resource.