Skip to content

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Oct 10, 2021

Adding template for opening any new PR, is there other suggestion what the template shall include?

@facebook-github-bot
Copy link
Contributor

Hi @Borda!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@datumbox
Copy link
Contributor

@Borda Thanks for the PR.

This seems to add information on how to submit a PR rather than standardize the way we submit PRs. Doesn't this mean that a people would have to delete most if not all the text upon submission?

@NicolasHug
Copy link
Member

NicolasHug commented Oct 11, 2021

Thanks for the PR @Borda

From my experience on other projects, this kind of PR checklist usually don't add much to the quality of PRs, but they do add a bit of unnecessary overhead to the submission process.

So for now I'm not convinced this would be a net improvement on the process. Perhaps your experience is different?

Maybe we can keep in super simple and just write something like

Before submitting a PR, please make sure to check our contributing guidelines regarding code formatting, tests, and documentation. Thanks for contributing!

with a link for each section

@Borda
Copy link
Contributor Author

Borda commented Oct 11, 2021

I do not insist in a checklist, but mainly I would add some section, as it is very helpful for any new contributor to know what he/she shall write in the PR to facilitate/simplify the review process

@datumbox
Copy link
Contributor

Perhaps this info is worth adding/clarifying elsewhere? I think that updating the template to add a message that needs to be removed by the submitter just adds additional barriers. This is unlike the ISSUE templates that indeed coordinate and organize the submission. Perhaps if following the same approach here could be beneficial.

@Borda
Copy link
Contributor Author

Borda commented Oct 11, 2021

Perhaps this info is worth adding/clarifying elsewhere? I think that updating the template to add a message that needs to be removed by the submitter just adds additional barriers. This is unlike the ISSUE templates that indeed coordinate and organize the submission. Perhaps if following the same approach here could be beneficial.

well for me the PR template is also a kind of crossroad/guide what I need to make to prepare good PR
so I would introduce some section, also if a PR fixes already reported/open issue
moreover you can cross-link to contribution guide

@datumbox
Copy link
Contributor

@Borda thanks for the thoughts.

@fmassa @prabhat00155 @pmeier if you have any thoughts on this let us know.

@prabhat00155
Copy link
Contributor

Thanks @Borda for the PR! I like the idea, but I do agree with @datumbox and @NicolasHug, perhaps we could have a guideline with such details than create a template.

@Borda
Copy link
Contributor Author

Borda commented Nov 9, 2021

Thanks @Borda for the PR! I like the idea, but I do agree with @datumbox and @NicolasHug, perhaps we could have a guideline with such details than create a template.

@prabhat00155 so what do you suggest as the next steps, leave it waiting for guideline?

@NicolasHug
Copy link
Member

I believe the guidelines that @prabhat00155 refers to is our existing contributing guide

I would just link to our contributing guide with a short sentence as suggested above #4586 (comment)

@Borda
Copy link
Contributor Author

Borda commented Nov 29, 2021

@NicolasHug mind have look at the latest update?

@NicolasHug
Copy link
Member

I made some comments @Borda but to be fair, I would prefer to keep this extremely and just have one single sentence that links to our contributing guide.

@Borda Borda requested a review from NicolasHug November 29, 2021 14:40
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @Borda , let's give this a try

@NicolasHug NicolasHug merged commit 93b26da into pytorch:main Dec 6, 2021
@NicolasHug NicolasHug added the other if you have no clue or if you will manually handle the PR in the release notes label Dec 6, 2021
@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

@Borda Borda deleted the gh-template/PR branch December 6, 2021 18:27
facebook-github-bot pushed a commit that referenced this pull request Dec 9, 2021
Summary:
* adding PR template

* update

* Update PULL_REQUEST_TEMPLATE.md

* Update PULL_REQUEST_TEMPLATE.md

* Update .github/PULL_REQUEST_TEMPLATE.md

Reviewed By: NicolasHug

Differential Revision: D32950939

fbshipit-source-id: f1334692a1f436e0a1ae00772637030ff1531b92

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/default cla signed other if you have no clue or if you will manually handle the PR in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants