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

Remove PR template #423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove PR template #423

wants to merge 1 commit into from

Conversation

mveytsman
Copy link
Member

Describe your changes

The PR template feels like make-work, and I know that I for one have not been using it / finding it helpful.

I think it's good to prompt for self-review and testing but this checklist doesn't feel like the right way to do it.

WDYT @teesloane @sereprz


THIS IS WHAT WILL NO LONGER BE IN PRs:

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added tests.
  • Are there other PRs or Issues that I should link to here?
  • Will this be part of a product update? If yes, please write one phrase
    about this update in the description above.

@sereprz
Copy link
Member

sereprz commented Nov 7, 2024

I think that the purpose of a template is to encourage to be explicit about what the PR is for and track decisions so I'm pro some template over no template. I think the 3 of us agreed on this when we were working on stuff that wasn't getting reviewed always?

This feels like a very lightweight format to me (it's just a checklist? 🤷‍♀️ ) - what would be a better way?

@mveytsman
Copy link
Member Author

mveytsman commented Nov 7, 2024

I see your point and I agree that being explicit about what a PR is for and tracking decisions is good. I will be totally honest "I have performed a self-review of my code" gives me a negative emotional reaction and makes me feel condescend to.

With that said, I don't think the checklist is serving it's purpose; I've looked through the past 10 merged PRs and

  • I have performed a self-review of my code - 8 checks
  • If it is a core feature, I have added tests. - 0 checks
  • Are there other PRs or Issues that I should link to here? - 3 checks
  • Will this be part of a product update? If yes, please write one phrase - 0 checks

The Are there other PRs or Issues that I should link to here? is covered by the comment in the description so it's superfluous. For example here's what I think is a good PR that only has the first item checked, even though it does reference the relevant issues, as in closes #346: #382 . Another thing I like about this PR is the screenshots, a practice I think should be encouraged, more on this later.

So of the 4 item checklist, one I feel called out by, one is already covered by a comment in the issue body, and two aren't used1. That's why I feel getting rid of the checklist is good user-experience.

If we want to encourage PRs that aren't empty and linking to issues to facilitate decision tracking, we can leave the original top-level part / I also wouldn't be opposed to adding a nudge to add screenshots, e.g.

## Describe your changes

<!-- Please add a link to a ticket if relevant: eg: "closes #340" -->

## Screenshots

<!-- Only include screenshots if it's a user-facing change -->

Footnotes

  1. I do think we should encourage writing of tests! It just seems this checklist isn't doing it, and also it's unclear what a "core feature" is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants