Added a template for PRs by ilyachur · Pull Request #4313 · openvinotoolkit/openvino

ilyachur · 2021-02-12T11:06:40Z

Details:

Added template for pull requests, please review this file and let me know if we need to update anything.
Feedbacks are welcome

Tickets:

NA

vinograd47 · 2021-02-12T11:32:11Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

@@ -0,0 +1,30 @@
+## PR Description: *short_description*


Doesn't it just duplicate the PR title?

Ok, I agree.

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

vinograd47 · 2021-02-12T11:33:43Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

+## PR Description: *short_description*
+
+### Tickets:
+ - *ticket-id*


Can we add tickets from private bug tracer here?

I think we can add the bug id

vinograd47 · 2021-02-12T11:37:05Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

+- [ ] I agree to contribute to the project under Apache 2 License.
+- [ ] To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenVINO
+- [ ] The PR is proposed to proper branch
+- [ ] There is reference to original bug report and related work


What if it is a new feature?

bug / feature can be selected via labels.

And sometimes it's hard to say - it's bug or feature :)

I updated the message

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

vinograd47 · 2021-02-12T11:38:03Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

+    - [ ] Unit tests
+    - [ ] Function tests
+    - [ ] e2e tests
+    - [ ] Component dependent tests


IMHO, not needed, since CI is launched automatically.

In this sentence I mean that contributor should think about tests which are needed for his PR.

vinograd47 · 2021-02-12T11:38:55Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

+    - [ ] User guide update
+    - [ ] Developer guide update
+    - [ ] Specification update
+    - [ ] Component specific guides


IMHO, if the PR relates to the Documentation it will be mentioned in its Title/Details section. I don't see the reason to duplicate this information in the Checklist.

If you add a new functionality you need to add a documentation for it. In this checklist contributor can think about different documentations and update if it is required.

For me the checklist looks like obligatory conditions. But developer don't need to update User guide if the PR modifies internal things.

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

GlebKazantaev · 2021-02-15T07:26:02Z

The purpose of PR description is to help reviewers to understand which problem it solves and what are the peculiarities of the solution. So as a reviewer I don't expect in PR to see some "checkboxes" that reminds developer not to forget to do something. All this rules that developer must follow must be located inside developer guide but bot in PR. So instead of searching for particular checkboxes I insist to work on our guidelines and make them clean and transparent for any developers so they can always rely on them and follow them but not checkboxes in PR that doesn't cover all aspects. So from my point of view the guidelines is the right way how we can grow our opensource/contribution culture.

The problem that developers do not put any description to PR or miss some parts (like tests, documentation, code-style etc.) can be solved if reviewers will point contributors to specific parts in contribution guideline or component specific guides which needs to be explained or implemented. And if all reviewers will follow this approach and put dev-guides in the center of development flow then in future all contributors will rely on them by default and the quality of PRs will increase.

Let's look at the changes you propose:

Details and Tickets section: looks good.

Check-list section:

License check-boxes: agreement to accept the license can be kept in contribution-guide so contributor accept them by default.
"The PR is proposed to proper branch": I think it is too obvious.
"There is reference to original request or bug report and related work" - in most cases this one will be empty. Also git-hub shows the link to PR if current PR was mentioned. So if such need exist I suggest to use git-hub functionality.
"The PR doesn't have backward incompatible changes" - the first candidate to contribution guide.
"The code is documented well" - contribution guide.
"All new files contain the license header" - this must be included into CI checks. I know that MO already has this checks.
"New tests, which are covered the functionality from PR, were added" - contribution guide.
"Documentation updates" - contribution guide.

So from my point of view only "Description" and "Tickets" sections make sense.

ilyachur · 2021-02-15T09:38:06Z

Thank you guys, I got feedback from you. I will update the PR and notify you.

ilyachur · 2021-02-15T11:03:03Z

I updated the template

.github/pull_request_template.md

Added a template for PRs

94547b2

ilyachur requested review from a team, GlebKazantaev, ababushk, azhogov, ilya-lavrenov, lazarevevgeny and slyalin February 12, 2021 11:06

Fixed style

14de169

openvino-pushbot added category: CI OpenVINO public CI category: Core OpenVINO Core (aka ngraph) category: docs OpenVINO documentation labels Feb 12, 2021

vinograd47 reviewed Feb 12, 2021

View reviewed changes

Changed the structure

433a947

vladimir-paramuzov reviewed Feb 12, 2021

View reviewed changes

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md Outdated Show resolved Hide resolved

ilyachur added 2 commits February 15, 2021 13:57

Fixed comments

fad6dc5

Merge remote-tracking branch 'upstream/master' into add_pr_template

dce8404

ilyachur requested review from vinograd47 and vladimir-paramuzov February 15, 2021 11:02

ilya-lavrenov approved these changes Feb 15, 2021

View reviewed changes

vladimir-paramuzov reviewed Feb 15, 2021

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

vinograd47 approved these changes Feb 15, 2021

View reviewed changes

Fixed comments

58b6461

vladimir-paramuzov approved these changes Feb 15, 2021

View reviewed changes

ilyachur merged commit c52c491 into openvinotoolkit:master Feb 15, 2021

ilyachur deleted the add_pr_template branch February 15, 2021 12:18

Conversation

ilyachur commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details:

Tickets:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GlebKazantaev commented Feb 15, 2021

Uh oh!

ilyachur commented Feb 15, 2021

Uh oh!

ilyachur commented Feb 15, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ilyachur commented Feb 12, 2021 •

edited

Loading