Skip to content

Provide some example on how to leave PR reviews #452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 4, 2019
Merged

Provide some example on how to leave PR reviews #452

merged 4 commits into from
Feb 4, 2019

Conversation

Mariatta
Copy link
Member

@Mariatta Mariatta commented Feb 4, 2019

No description provided.

eamanu
eamanu reviewed Feb 4, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if the PR is about a typo, or a fix documentation, or something that you don't need to test or don't have any that you consider an improve?

@Mariatta
Copy link
Member Author

Mariatta commented Feb 4, 2019

For documentation, sometimes it is still worth rendering the docs.

For real typos, at the very least, you can leave comment such as "assert is indeed the right spelling, not assret". You can also provide more context such as "I've confirmed that this is the only place where this typo happened".

terryjreedy
terryjreedy requested changes Feb 4, 2019
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. Review is our bottleneck, and many contributors are reluctant because they don't know how or don't think it helpful.

pullrequest.rst Outdated
2. Whether the PR fixes the linked bug/issue

3. If you requested changes to the pull request, provide suggestions on how it
can be improved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly If you request changes, try to suggest how.

terryjreedy
terryjreedy reviewed Feb 4, 2019
View reviewed changes
pullrequest.rst Outdated

1. Whether you have tested the PR

2. Whether the PR fixes the linked bug/issue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be consolidated with an important item added.

`1. If you tested the PR, report the result and the system and version tested on, such as 'Windows 10', 'Ubuntu 16.4', or 'Mac High Sierra'.

@Mariatta
Copy link
Member Author

Mariatta commented Feb 4, 2019

Thanks @terryjreedy! I've incorporated your suggestions.

@Mariatta Mariatta merged commit bc000ac into master Feb 4, 2019
@Mariatta Mariatta deleted the Mariatta-patch-2 branch February 4, 2019 22:59
@Mariatta
Copy link
Member Author

Mariatta commented Feb 4, 2019

Thanks!

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
@Mariatta
Provide some examples on how to leave PR reviews (python#452)
7ffccc8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryjreedy terryjreedy terryjreedy approved these changes

+1 more reviewer

@eamanu eamanu eamanu left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mariatta @eamanu @terryjreedy @the-knights-who-say-ni