[Merge/release] Add commit message linting feature #43
Conversation
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 <alexellis2@gmail.com>
GET.md
Outdated
| Commit linting ensures: | ||
|
|
||
| * subjects start with a capital letter | ||
| * subject lines are less than 50 characters |
There was a problem hiding this comment.
Less than or equal to 50 characters
There was a problem hiding this comment.
GitHub will allow up to 72 characters before it wraps. I'm wondering whether we should expand the limit?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I could move the hard-limit to 72 characters as per GitHub UI
There was a problem hiding this comment.
subject lines contain no more than 72 characters
pullRequestHandler.go
Outdated
| var valid bool | ||
|
|
||
| if message == nil { | ||
| return false |
There was a problem hiding this comment.
As this is tested in lintCommits, is this belt and braces or a hangover?
There was a problem hiding this comment.
I'm not sure.. I might have to double-check
pullRequestHandler.go
Outdated
| } | ||
| 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 + `): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 <alexellis2@gmail.com>
|
@rgee0 FYI I've pushed some changes - see commit message. |
alexellis
changed the title
Signed-off-by: Alex Ellis <alexellis2@gmail.com>
Fixes: #43
Adds feature to .DEREK.yml
commit_linting, which when enabledlints 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 alexellis2@gmail.com