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.

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

Created pull request template #225

Merged
merged 7 commits into from
Sep 23, 2021
Merged

Conversation

francescalb
Copy link
Collaborator

@francescalb francescalb commented Sep 22, 2021

Created a pull request template in the .github folder.

This is intended to be used by the person creating the pull request prior to asking for review and as a help for the reviewer.

It addresses issue 190, which asks for a checklist for the reviewer. This pull request makes #191 redundant.

@francescalb francescalb linked an issue Sep 22, 2021 that may be closed by this pull request
Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

By wrapping lines in <!-- this is an HTML comment ---> they become invisible when creating the PR, even though they have not been removed from the text.

It might be nice to underline that the user making the PR should not interact with the checklist whatsoever, but that it's a reviewer-only feature?

Hmm... actually, the checklist part shouldn't really be in the PR template, but rather it should be a template for the review response (like this one 😉) - also the "Comments" section. But unfortunately, I don't think GitHub supports those kind of templates :|

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@francescalb
Copy link
Collaborator Author

Yes, initally I wanted a template for the reviewer, but github does not support that as far as I can see. I think this should be an OK help for both reviewer and developer like this, and I think it is better to have this here, than having a separate checklist somewhere else that both reviewer and requester might easily forget.

francescalb and others added 6 commits September 22, 2021 17:57
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Looks fine to me.
There might still be a bit of confusion on my end about why the person opening a PR needs to fill out the checklist (as it's written now), since this should be the reviewer's task, but it's minor.

@CasperWA
Copy link
Collaborator

As a note, you can combine the suggested changes you want to implement from a review and apply/commit them as a batch (all in one) :)

@francescalb
Copy link
Collaborator Author

Looks fine to me.
There might still be a bit of confusion on my end about why the person opening a PR needs to fill out the checklist (as it's written now), since this should be the reviewer's task, but it's minor.

I kind of agree, this way there is a bit more pressure on the person opening the PR to do it well. Let us try this and see how it works.

@francescalb francescalb merged commit 2366379 into master Sep 23, 2021
@CasperWA CasperWA deleted the flb/pull_request_template branch March 2, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formalize review process with checklists
2 participants