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

Fix good commit example to use imperative title #587

Merged
merged 5 commits into from May 25, 2020

Conversation

@DahlitzFlorian
Copy link
Contributor

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

- Fix example to use imperative title
- Explicitly mention the preference for imperative over descriptive titles
- Link to the article from Chris Beams
Copy link
Member

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

pullrequest.rst Outdated Show resolved Hide resolved
@DahlitzFlorian
Copy link
Contributor Author

@DahlitzFlorian DahlitzFlorian commented May 22, 2020

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

Copy link
Member

@terryjreedy terryjreedy left a comment

Approve other than one case issue.

pullrequest.rst Outdated Show resolved Hide resolved
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@DahlitzFlorian DahlitzFlorian requested review from terryjreedy and aeros May 24, 2020
@terryjreedy
Copy link
Member

@terryjreedy terryjreedy commented May 24, 2020

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

pullrequest.rst Outdated Show resolved Hide resolved
Co-authored-by: Kyle Stanley <aeros167@gmail.com>
@aeros
aeros approved these changes May 25, 2020
Copy link
Member

@aeros aeros left a comment

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

@aeros aeros merged commit 74d5f3a into python:master May 25, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docs/readthedocs.org:cpython-devguide Read the Docs build succeeded!
Details
@DahlitzFlorian DahlitzFlorian deleted the DahlitzFlorian:fix-issue-577 branch May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.