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

Mention commit messages in the contribution guidelines. #5504

Merged
merged 12 commits into from
Sep 10, 2021

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Aug 6, 2021

Description

@stefanv as per the recent email thread about commit messages.

I wasn't 100% sure about linking to the reference @alexdesiqueira and you shared, at least in writing (I think it's okay to hear misnomers, but less okay to read them): In the first paragraph, I suspect that "Well-structured and well-written git commits" stands for "Well-structured git commits and well-written git commit messages" and, slightly later, I wonder whether "writing quality git commits" would stand for "writing quality git commit messages."

Actually, I don't think "writing commits" would be incorrect (that would be writing everything the commit consists of, not only its message). Since this talk by Rose Judge deals with commits overall (not just commit messages---which is also the whole point of the talk), it's not 100% clear to me whether "writing quality git commits" is just short for "writing quality git commit messages."

Anyhow, I went reductionist and decided to link to the good ol' post by Chris Beams on just writing commit messages (which inevitably impacts how you structure actual commits anyway). But I'm happy to update the PR if the majority prefers the other reference.

I removed the part about merge messages since we now 'squash and merge.'

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@mkcor mkcor added the 📄 type: Documentation Updates, fixes and additions to documentation label Aug 6, 2021
@stefanv
Copy link
Member

stefanv commented Aug 6, 2021

Thank you, Marianne. I think Chris's post is great. Unfortunately, it's also a bit of a slog to get through. I like this summary from Charl Botha's software development guide: https://vxlabs.com/software-development-handbook/#good-commit-messages

Perhaps we should include a similar summary, or link to his?

I am worried that this tiny sentence will get lost in the guidelines. Perhaps we should also update our pull request template to emphasize it?

@mkcor
Copy link
Member Author

mkcor commented Aug 14, 2021

All very good points, @stefanv. This summary from Charl Botha's software development guide is great and very fitting, indeed. And it starts with a link to Chris's post, so I've happily made the change.

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks @mkcor

We may also want to mention somewhere in the maintainer guide about editing the messages a bit when squashing the commits. I often delete or condense a number of the less informative ones like "pep8 fix", "apply review comments", etc, while making sure to maintain any Co-authored-by entries.

@mkcor
Copy link
Member Author

mkcor commented Aug 28, 2021

@grlee77 good point! Done 13e5ae5.

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Small tweak, since I don't think most people know what "standard" commit messages are. Otherwise I think this is good to go!

CONTRIBUTING.txt Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
mkcor and others added 2 commits September 8, 2021 10:58
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
@mkcor
Copy link
Member Author

mkcor commented Sep 8, 2021

Thanks, @stefanv! Your latest review allowed me to catch a leftover typo (c74c3bf).

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

I did a half-baked review yesterday. Apologies for the churn, @mkcor!

doc/source/core_developer.md Outdated Show resolved Hide resolved
CONTRIBUTING.txt Outdated Show resolved Hide resolved
mkcor and others added 5 commits September 8, 2021 19:46
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
@stefanv
Copy link
Member

stefanv commented Sep 8, 2021

Looks good to me; thanks @mkcor!

@mkcor
Copy link
Member Author

mkcor commented Sep 9, 2021

Looks good to me; thanks @mkcor!

@stefanv great! Please go ahead and merge 😅

@grlee77 grlee77 merged commit dc4a67e into scikit-image:main Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants