Skip to content

Fix good commit example to use imperative title - #587

Merged
aeros merged 5 commits into
python:masterfrom
DahlitzFlorian:fix-issue-577
May 25, 2020
Merged

Fix good commit example to use imperative title#587
aeros merged 5 commits into
python:masterfrom
DahlitzFlorian:fix-issue-577

Conversation

@DahlitzFlorian

@DahlitzFlorianDahlitzFlorian commented May 21, 2020

Copy link
Copy Markdown
Contributor

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
@gvanrossum
gvanrossum requested a review from aerosMay 21, 2020 21:07
aeros
aeros reviewed May 22, 2020
View reviewed changes

@aerosaeros left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Comment threadpullrequest.rst Outdated
@DahlitzFlorian

DahlitzFlorian commented May 22, 2020

Copy link
Copy Markdown
ContributorAuthor

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

terryjreedy
terryjreedy requested changes May 22, 2020
View reviewed changes

@terryjreedyterryjreedy left a comment

Copy link
Copy Markdown
Member

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.

Comment threadpullrequest.rst Outdated
@DahlitzFlorian@terryjreedy
Apply suggestion made by@terryjreedy
7d16a28
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@terryjreedy

terryjreedy commented May 24, 2020

Copy link
Copy Markdown
Member

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

aeros
aeros reviewed May 25, 2020
View reviewed changes
Comment threadpullrequest.rst Outdated
@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

@aerosaeros left a comment

Copy link
Copy Markdown
Contributor

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.

@aeros
aeros merged commit 74d5f3a into python:masterMay 25, 2020
@DahlitzFlorian
DahlitzFlorian 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

None yet

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