◐ Shell
reader mode source ↗
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

@DahlitzFlorian DahlitzFlorian 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 aeros May 21, 2020 21:07
aeros
aeros reviewed May 22, 2020
View reviewed changes

@aeros aeros 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.

Hide comment

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

DahlitzFlorian commented May 22, 2020

Copy link
Copy Markdown
Contributor Author

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

terryjreedy
terryjreedy requested changes May 22, 2020
View reviewed changes

@terryjreedy terryjreedy 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.

Hide comment

Approve other than one case issue.

@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.

@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

@aeros aeros 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.

Hide comment

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

@aeros aeros merged commit 74d5f3a into python:master May 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy Awaiting requested review from terryjreedy

1 more reviewer

@aeros aeros aeros 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