Skip to content

Fix good commit example to use imperative title#587

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.

Sign up for GitHub

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

Jump to bottom
Merged
aeros merged 5 commits into from
May 25, 2020
Merged

Fix good commit example to use imperative title #587

aeros merged 5 commits into from
May 25, 2020

Conversation

@DahlitzFlorian
Copy link
Contributor

@DahlitzFlorianDahlitzFlorian commented May 21, 2020

Closes: #577

  • Fix good commit example to use imperative title
  • Explicitly mention the preference for imperative over descriptive titles
  • Link to the article from Chris Beams

@gvanrossum I added the link to the article as suggested by you. The Git Bootcamp is mentioning the article, too, but I agree with you that it is better to explicitly mention it at this point, too.

@DahlitzFlorian
Merge remote-tracking branch 'upstream/master'
8b5876f
@DahlitzFlorian
Fix good commit example to use imperative title
e6432e1
- Fix example to use imperative title - Explicitly mention the preference for imperative over descriptive titles - Link to the article from Chris Beams
@gvanrossumgvanrossum requested a review from aerosMay 21, 2020 21:07
aeros
aeros reviewed May 22, 2020
View reviewed changes
Copy link
Contributor

@aerosaeros left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR @DahlitzFlorian. It mostly LGTM; I just have a very minor suggestion.

Since this issue is very well defined and straightforward, I'm good w/ merging it after the suggested changes are made.

@DahlitzFlorian
Copy link
ContributorAuthor

DahlitzFlorian commented May 22, 2020

Thanks for the fast feedback @aeros! I applied your suggestion.

terryjreedy
terryjreedy requested changes May 22, 2020
View reviewed changes
Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

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

Approve other than one case issue.

@DahlitzFlorian@terryjreedy
Apply suggestion made by@terryjreedy
7d16a28
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@terryjreedy
Copy link
Member

terryjreedy commented May 24, 2020

LGTM. Kyle, leaving this for you if you get to it.

aeros
aeros reviewed May 25, 2020
View reviewed changes
@DahlitzFlorian@aeros
Apply changes suggested by@aeros
2be1ce9
Co-authored-by: Kyle Stanley <aeros167@gmail.com>
aeros
aeros approved these changes May 25, 2020
View reviewed changes
Copy link
Contributor

@aerosaeros left a comment

Choose a reason for hiding this comment

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

With the latest changes, LGTM. Thanks for working on this @DahlitzFlorian.

@aerosaeros merged commit 74d5f3a into python:masterMay 25, 2020
@DahlitzFlorianDahlitzFlorian deleted the fix-issue-577 branch May 25, 2020 11:33
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
@DahlitzFlorian
Fix good commit example to use imperative title (pythonGH-587)
7f60eef
* Explicitly mention the preference for imperative over descriptive titles * Link to the article from Chris Beams
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedyterryjreedyAwaiting requested review from terryjreedy

1 more reviewer

@aerosaerosaeros approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use imperative for commit title; fix example

4 participants

@DahlitzFlorian@terryjreedy@aeros@the-knights-who-say-ni