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

[Merge/release] Add commit message linting feature #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alexellis
Copy link
Owner

@alexellis alexellis commented Mar 18, 2018

Fixes: #43

Adds feature to .DEREK.yml commit_linting, which when enabled
lints commit messages with a style consistent with the guidelines
written up by Chris Beams [1]. The linting will place a label on a PR
and send a comment when it fails linting, but this is not blocking.

The label used is: review/commit-message and that will be removed
if and when the linting is fixed.

Unit test coverage is added for the linting.

The DCO checking feature was also tested and an optimization was
added so that the PR labels are only fetched once and then re-used
this is to save on API rate-limiting and server trips.

[1] https://chris.beams.io/posts/git-commit/

The two guidelines implemented are referenced in the GET.md file (documentation)

Signed-off-by: Alex Ellis [email protected]

Adds feature to .DEREK.yml `commit_linting`, which when enabled
lints commit messages with a style consistent with the guidelines
written up by Chris Beams [1]. The linting will place a label on a PR
and send a comment when it fails linting, but this is not blocking.

The label used is: review/commit-message and that will be removed
if and when the linting is fixed.

Unit test coverage is added for the linting.

The DCO checking feature was also tested and an optimization was
added so that the PR labels are only fetched once and then re-used
this is to save on API rate-limiting and server trips.

[1] https://chris.beams.io/posts/git-commit/

Signed-off-by: Alex Ellis <[email protected]>
GET.md Outdated
Commit linting ensures:

* subjects start with a capital letter
* subject lines are less than 50 characters
Copy link
Contributor

Choose a reason for hiding this comment

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

Less than or equal to 50 characters

Copy link
Owner Author

Choose a reason for hiding this comment

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

GitHub will allow up to 72 characters before it wraps. I'm wondering whether we should expand the limit?

Copy link
Contributor

Choose a reason for hiding this comment

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

In all areas that the subject is displayed? Is the client similar? The referenced article is 4 years old so gh may have changed in that time.

I liked the idea of that being configurable, even if it was through env-vars.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I could move the hard-limit to 72 characters as per GitHub UI

Copy link
Contributor

Choose a reason for hiding this comment

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

subject lines contain no more than 72 characters

var valid bool

if message == nil {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is tested in lintCommits, is this belt and braces or a hangover?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not sure.. I might have to double-check

}
link := fmt.Sprintf("https://github.com/%s/%s/blob/master/CONTRIBUTING.md", req.Repository.Owner.Login, req.Repository.Name)

body := `Please check that your commit messages fit within [these guidelines](` + link + `):

Choose a reason for hiding this comment

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

Please check that your commit messages fit within [these guidelines](link):

  • Commit subject should not exceed 50 characters
  • Commit subject should start with an uppercase letter

I think there needs to be a quick link here to make it as easy as possible for everyone to edit their commit history. A direct link to this page (as in the contributing guide) would be perfect.

Prior Slack discussion here.

Thoughts?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes a link could be good for re-writing history.

GitHub's UI will allow 72 chars so I've changed the code to use
that as a hard limit instead of 50. The message still prefers 50
characters.

Also implements linting to prevent punctuation of (.!) given at
end of subject line.

Signed-off-by: Alex Ellis <[email protected]>
@alexellis
Copy link
Owner Author

@rgee0 FYI I've pushed some changes - see commit message.

@alexellis alexellis changed the title Add commit message linting feature [Merge/release] Add commit message linting feature Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants