dwyl / contributing

:clipboard: Guidelines & Workflow for people contributing to our project(s) on GitHub. Please :star: to confirm you've read & understood! :white_check_mark:
86 stars 9 forks source link

Process - Review or Assign? #86

Closed ghost closed 7 years ago

ghost commented 7 years ago

For the following line:

https://github.com/dwyl/contributing/blob/master/README.md#3-pr-ettiquete

It's not clear if one should use the Github Assign feature or the Github Reviewer feature

What is the correct process to follow? I assume using the review feature is enough without also additionally Assigning the same reviewer

@nelsonic ?

nelsonic commented 7 years ago

@markwilliamfirth yes, the Review feature is a recent development. Assigning a PR or Requesting a Review can be used interchangeably. However I would say that while we can Request Review from multiple people we should really only assign it to one person (at a time) so it's clear who's responsibility the work is. If a person is Requesting Review do they need to also assign the PR to the same person? No, that feels like overkill to me. (an extra step without much value)

The only thing to note is: if we use the Request Review to ask a specific person for review, if the person asks for "modifications" to be made to the PR before it can be merged, it will still look like the PR is being reviewed as opposed to being the "responsibility" of the person who prepared the PR ... for example: pull-request-reviewer-assigned I have reviewed a PR for a project but requested changes, I cannot "remove" myself from the reviewers list but I can assign it back to the person who created the PR ... The workflow does work, but it requires some explaining...

mk4111 commented 7 years ago

...so @nelsonic does that mean we should try and 'Assign' over 'Review'...?

iteles commented 7 years ago

@mk4111 Until github fixes its interface, that's the preference 👍

iteles commented 7 years ago

Closing as for now the readme read 'assign your PR'

screen shot 2017-07-30 at 18 49 48

Will reopen if required