Skip to content

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.

3 participants