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

Update Github pull_request_template.md to include README update, remove Task section #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timirwin
Copy link
Member

Why?

Why were the changes needed? What issues were the changes addressing?
(Note: some changes may seem unrelated to the ticket, this is a great place to explain further.)

Linear automatically links to the PR when referenced in the PR name; READMEs are hard to remember to update to confirm they get updated upon PR creation/review

What Changed

What changed in this PR?

  • remove Task reference from pull_request_template generator
  • add README update pre-merge checklist to pull_request_template generator
  • add/remove from rolemodel_rails pull_request_template.md too

Screenshots

If any UI changes need to be shown off, please add screenshots here.

Copy link
Member

@reedrolemodel reedrolemodel left a comment

Choose a reason for hiding this comment

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

I like the Task section removal because it's redundant with Linear integration. Do you add it to the PR title as a prefix? The only advantage there is visibility in git log, which I don't think I ever use.

@timirwin
Copy link
Member Author

I like the Task section removal because it's redundant with Linear integration. Do you add it to the PR title as a prefix? The only advantage there is visibility in git log, which I don't think I ever use.

I added it as a suffix. Is that good enough, @reedrolemodel?

@timirwin timirwin changed the title Update Github pull_request_template.md to include README update, remove TR Update Github pull_request_template.md to include README update, remove Task section Nov 22, 2024
@Jeremy-Walton
Copy link
Member

The format that we have typically aimed for is described here. https://github.com/RoleModel/BestPractices/blob/master/git/commit-messages.md

I've always done [APP#xxx] Task Title (GH#xx)

@@ -15,6 +11,10 @@ What changed in this PR?
* [ ] Change 2
* [ ] ...

Choose a reason for hiding this comment

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

Thoughts on removing these extras? I always need to delete them and add my own

Copy link
Member

Choose a reason for hiding this comment

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

Hitting Enter after a todo adds a new one automatically, so I agree these are unnecessary.

Copy link
Member

@OutlawAndy OutlawAndy Nov 22, 2024

Choose a reason for hiding this comment

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

though, I guess this is the important file to make the change in. Isn't it?

Comment on lines 11 to 12
* [ ] Change 2
* [ ] ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [ ] Change 2
* [ ] ...

Copy link
Member

Choose a reason for hiding this comment

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

apply to generator template also

@reedrolemodel
Copy link
Member

I like the Task section removal because it's redundant with Linear integration. Do you add it to the PR title as a prefix? The only advantage there is visibility in git log, which I don't think I ever use.

I added it as a suffix. Is that good enough, @reedrolemodel?

My point is I don't need it. The Github PR number is appended by Squash and merge. If I need to look up the task, I can follow the link in my git client to the PR and then follow the link to Linear from there.

@reedrolemodel
Copy link
Member

The format that we have typically aimed for is described here. https://github.com/RoleModel/BestPractices/blob/master/git/commit-messages.md

I've always done [APP#xxx] Task Title (GH#xx)

That's how we do it too, except the PR number is appended automatically by Squash merges. For example:

556d2507 * [TS#845] Skip SSN presence validation if archived (#1624)
d19bf617 * [TS#841] Fix invitation flow bypass when resetting password (#1623)
114b2a0d * [TS#843] Complete year button (#1621)

Copy link
Member

@OutlawAndy OutlawAndy left a comment

Choose a reason for hiding this comment

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

+1 for removing task list items after Change 1. Otherwise :shipit:

Comment on lines 11 to 12
* [ ] Change 2
* [ ] ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [ ] Change 2
* [ ] ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants