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

doc : add pull request template #61

Merged

Conversation

purohitdheeraj
Copy link
Contributor

Description

Summary: this PR introduces pull request template file

Fixes #58

Type of change

  • 📝 Documentation Update

Added tests?

  • 🙅 no, because they aren't needed

would request @nmn for feedback , will update acc

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 6, 2023
@nmn nmn requested a review from necolas December 6, 2023 11:48
@nmn
Copy link
Contributor

nmn commented Dec 6, 2023

Looks good to me.

@necolas Thoughts on this?

@kevintyj
Copy link
Contributor

kevintyj commented Dec 6, 2023

I personally prefer a more objective oriented and cleaner PR templates like this:

## What changed / motivation ?

## Linked PR/Issues

## Additional Context
<!--- Screenshots, Tests, Breaking Change, Anything Else ? --->

Then either have a preflight checklist or pre-merge checklist like so:

## Pre-flight checklist 
- [ ] I have read the contributing guidelines [LINK GOES HERE](url)
- [ ] I have signed the contributing agreements
- [ ] I have written unit tests where necessary (If it is a feature commit)
- [ ] Performed a self-review of my code
- [ ] More can come here depending on the details of the contributing guidelines...

The sample for this PR would look something like:

What changed / motivation ?

Adds a PR template for new PRs, adds consistency for contributors

Linked PR/Issues

#58

Additional Context

None

Pre-flight checklist

  • I have read the contributing guidelines LINK GOES HERE
  • I have signed the contributing agreements
  • I have written unit tests where necessary (If it is a feature commit)
  • Performed a self-review of my code
  • Made sure PR title follows the conventional commit specification

More info:
The title should hold the information about Type of change this PR is like so:
docs: Add PR template
Also linters for the PR title can be used (like commitlint) to enforce these rules using GitHub Actions.
Having a pre-flight checklist just be a simple checklist allows maintainers to quickly identify if checklists are met quickly in the PR list.

@purohitdheeraj
Copy link
Contributor Author

thanks @kevintyj for detailed feedback
and yes your PR looks clean and on point , going with that 🚀

@necolas
Copy link
Contributor

necolas commented Dec 6, 2023

@nmn I haven't used PR templates before. I guess I'm a little concerned that having to jump through a lot of hoops makes submitting PRs more tedious, especially for repeat contributors. I don't know that templates make PRs any better

@purohitdheeraj
Copy link
Contributor Author

Hi @necolas, I agree with you. We could opt for a simpler structure, like in other repos. I think having a PR template can serve as a helpful reminder for adhering to conventions and standards in case someone unintentionally misses them. What's your take on this? we can just keep the most imp stuff there

@nmn
Copy link
Contributor

nmn commented Dec 6, 2023

I share the concerns expressed by @necolas as well. I don't want to put up hurdles to PRs, so I'll leave this PR unmerged for now.

@nmn nmn added the question Further information is requested label Dec 6, 2023
@nestorvanz
Copy link
Contributor

IMO, PR templates should be easy to fill. Let's find a right balance here.

In the other hand, are there any other templates we know are useful?

@purohitdheeraj
Copy link
Contributor Author

Hi @nmn did the changes and shifted the template file to .github/PULL_REQUEST_TEMPLATE.md from .github/PR_TEMPLATE/pull_request_template.md

@nmn
Copy link
Contributor

nmn commented Dec 9, 2023

Going to merge to try it out for a while. We can revert if we hate it!

@nmn nmn merged commit 90dea1b into facebook:main Dec 9, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc : add pull request template
6 participants