Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

chore(process): add structure guide for PRs#62719

Merged
jhchabran merged 2 commits into
mainfrom
jh/update-pr-template
May 16, 2024
Merged

chore(process): add structure guide for PRs#62719
jhchabran merged 2 commits into
mainfrom
jh/update-pr-template

Conversation

@jhchabran

@jhchabran jhchabran commented May 16, 2024

Copy link
Copy Markdown
Contributor

Provides guidance to cover the basics with pull request descriptions.

Before there wasn't any and the quality would vary a ton between authors. Now, with that in place, it's a visible reminder to cover the basics and mitigate the blank page syndrome.

I created a how-to in Notion, blank for now that we can use to provide additional context.

Test plan

CI

@cla-bot cla-bot Bot added the cla-signed label May 16, 2024
@jhchabran jhchabran requested review from a team, olafurpg and vdavid May 16, 2024 08:22
Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated
-->

Fixes TODO
Before TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fixes TODO is an easy enough prompt to complete. But Before and After I don't immediately know what I would put there. Sure describe what is was before, and what it was after - but I got that after pondering a bit about what one would write there, which leads me to think; should we use different prompts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe I am alone in this - but we already have a lot of filler text to delete so that all the markdown comments do not end up in the commit. Should we rather just have a link at the top with How to write a good PR - see https://www.notion.so/.......

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to keep these instructions short, I think we could cover examples in the linked Notion how-to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's in a comment, why would you want to delete it? I personally never do so. I see your point though, but we all know that the path of least resistance will be taken in most cases, so it'll be super easy to not click on that link and just do as usual.

Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated
-->

Fixes TODO
Before TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am worried people will stick too rigorously to this template. This structure isn't ideal for all PRs. I would prefer to describe this structure as a comment. I also prefer "Previously" and "Now" over Before/After. It's more important that we increase the quality of code reviews, and push back more actively when people use bad descriptions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, will update in a minute.

@jhchabran jhchabran requested review from burmudar and olafurpg May 16, 2024 08:36
@jhchabran jhchabran merged commit 98c0516 into main May 16, 2024
@jhchabran jhchabran deleted the jh/update-pr-template branch May 16, 2024 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants