Skip to content
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

Instructions on documenting code style modifications #2109

Merged
merged 1 commit into from
Apr 18, 2021
Merged

Instructions on documenting code style modifications #2109

merged 1 commit into from
Apr 18, 2021

Conversation

felix-hilden
Copy link
Collaborator

From #2106: @ichard26 I know, I know you said you'd end up taking care of it yourself, but it's such an easy addition.

This PR adds a section to CONTRIBUTING.md where any style changes should be documented in the Black code style document. Probably another skip news?

Any wording improvements welcome, and I'll leave this open for modification in case you'd like to finish it yourself! Or if you had something else in mind, feel free to close this outright.

@ichard26 ichard26 added skip news Pull requests that don't need a changelog entry. T: documentation Improvements to the docs (e.g. new topic, correction, etc) labels Apr 15, 2021
ichard26
ichard26 approved these changes Apr 15, 2021
View reviewed changes
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

From #2106: @ichard26 I know, I know you said you'd end up taking care of it yourself, but it's such an easy addition.

Well, there's a reason why I put "probably" in my wording :)

another skip news?

mhmm you got it!

SGTM. Many thanks for picking this up and being so easy and fun to work with!

@felix-hilden
Copy link
Collaborator Author

Much appreciated 😄 but just so you know... you started it (hehe). But in all seriousness, thanks for being so open to work with us newbies!

@felix-hilden
Copy link
Collaborator Author

felix-hilden commented Apr 18, 2021 •
edited
Loading

Just so that I'm on the same page: are we waiting for another maintainer to review this? Or what's the pr-review-approve-merge etiquette of Black?

@JelleZijlstra JelleZijlstra merged commit 7b4ca4b into psf:master Apr 18, 2021
@ichard26
Copy link
Collaborator

ichard26 commented Apr 18, 2021 •
edited
Loading

what's the pr-review-approve-merge etiquette of Black?

Honestly, no clue! I'm the newest maintainer and we're quite informal around here. Any sort of "etiquette" around here is just implicitly agreed upon and does change. There are a few things that I've noticed though that may be informative:

  1. One approval by a maintainer is enough
    • Although each maintainer has a field they review best (e.g. documentation is my cup of tea), so it's better if the PR is
      approved (or at least looked at) by them.
  2. If there's any "requesting changes" reviews from a maintainer, they must be addressed before merge
  3. If you want others to wait for a review from you, "self-requesting" a review may or may not be enough (explicitly saying "HEY, I wanna review this, please wait" is best)
  4. We do wait before merging a PR once it's been approved to give others a chance to raise any comments

@felix-hilden
Copy link
Collaborator Author

I see, many thanks! And for the merge as well 🎉

@felix-hilden felix-hilden deleted the contributing-style-changes-doc branch April 22, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ichard26 ichard26 ichard26 approved these changes

Assignees
No one assigned
Labels
skip news Pull requests that don't need a changelog entry. T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felix-hilden @ichard26 @JelleZijlstra