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

chore: improve PR template text #3003

Merged
merged 3 commits into from
Oct 15, 2021
Merged

chore: improve PR template text #3003

merged 3 commits into from
Oct 15, 2021

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Oct 14, 2021

Improve PR template text

What is changing?

The PR template description in order to add helpful information and easy reference to important guides.

What is the motivation for this change?

It helps keep us organized, and double checks our essentials before publishing a PR.

Double check the following

  • Ran npm run check:lint script
  • Completed self-review using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes sufficiently covered by new or existing tests
    • n/a, markdown changes only
  • New TODOs have a related JIRA ticket
    • n/a, none

@nbbeeken nbbeeken force-pushed the chore/pr-template branch 2 times, most recently from 972779e to 74d6c83 Compare October 14, 2021 20:59
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Just a couple thoughts on phrasing here

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from kmahar October 14, 2021 21:15

#### Why does this need changes? Bug? or Feature?

##### 🙁 Actual behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth noting that this section and the next only apply to bug fix PRs?

For feature PRs it might be worth asking about like the use case for the feature or something like that, to help us evaluate the value of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into what I mentioned during our meeting about a button that differentiates the text based on a type. And that appears to be for Issues and not PRs. Maybe we drop these to headers. and just stick with #### What is the motivation for this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's reasonable, I think we could also put in the commented text something along the lines of "if filing a bug, please describe the current/desired behavior"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Published changes after I commented so now it has comments like so:

#### What is the motivation for this change?

<!-- If this is a bug, it helps to describe the current behavior and a clear outline of the expected behavior -->
<!-- If this is a feature, it helps to describe the new use case enabled by this change -->

@nbbeeken nbbeeken requested a review from kmahar October 15, 2021 17:59
@dariakp dariakp merged commit 0a66124 into 4.1 Oct 15, 2021
@dariakp dariakp deleted the chore/pr-template branch October 15, 2021 20:38
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants