README: Split out Commit Style notes into an expanded section. #1055
Conversation
|
Thanks for doing this, @neiljp, the section looks good!
|
There was a problem hiding this comment.
@neiljp Thanks for pushing this in. It looks good! 👍 I have left two minor concerns as in-line comments. Re the commit body, I believe we pretty much follow the Zulip version control guidelines, so a link to the docs would be sufficient.
|
|
||
| If you plan to submit git commits in pull-requests (PRs), then we highly suggest installing the `gitlint` commit-message hook by running `gitlint install-hook` (or `pipenv run gitlint install-hook` with pipenv setups). While the content still depends upon your writing skills, this ensures a more consistent formatting structure between commits, including by different authors. | ||
|
|
||
| If the hook is installed as described above, then after completing the text for a commit, it will be checked by gitlint against the style we have set up, and will offer advice if there are any issues it notices. If gitlint finds any, it will ask if you wish to commit with the message as it is (`y` for 'yes'), stop the commit process (`n` for 'no'), or edit the commit message (`e` for 'edit'). | ||
|
|
||
| Other gitlint options are available; for example it is possible to apply it to a range of commits with the `--commits` option, eg. `gitlint --commits HEAD~2..HEAD` would apply it to the last few commits. | ||
|
|
||
| **NOTE** Not all style suggestions are identified by gitlint at this time, including: | ||
| * If modifying code (not just tests), list modified filenames without extensions between slashes in only one area of the commit title (eg. `run/model/core/README: Some description.`) | ||
| * If modifying tests in addition to code, just note this in the body of the commit after a blank line (eg. `Tests updated.`) |
There was a problem hiding this comment.
We don't check for this in the GitLint, right? Should we keep it here in the README?
There was a problem hiding this comment.
Some of these are mentioned in the new section, so how about adjusting the trailing sentence of that from:
To make it easier to meet these style guidelines we recommend installing GitLint - see the next section!
to
To aid in satisfying some of these rules you can use `GitLint`, as described in the following section.
**However**, please check your commits manually versus these style rules, since GitLint cannot check everything - or language or grammar!
|
|
||
| Our commit titles have slight variations from the general Zulip style, with each: | ||
| * starting with one or more areas in lower case, followed by a colon and space | ||
| * area being slash-separated modified files without extensions, or the type of the change |
There was a problem hiding this comment.
Should we recommend filenames (and areas) in an alphabetically sorted fashion, so that the git log is more consistent?
There was a problem hiding this comment.
I know that some contributors do this, but there are various rules we could use for ordering, and I think it's best to not impose even more rules on contributors in this way? I'm not sure what we gain in alphabetical ordering that isn't achievable with eg. some kind of grep. We could sort by "size of change in file contents" or others - and they all impose various extra work which can be determined otherwise.
Instead we could explicitly mention that the order doesn't matter?
(listing files already impose extra burden, but help summarize what has changed quickly in the log titles - and too many entries sometimes indicates if a commit is too complicated - if it's not a refactor, for example)
There was a problem hiding this comment.
Right, that does add a bit of resistance; seems reasonable. 👍 We could mention that the order doesn't matter if needed later.
There was a problem hiding this comment.
as per PM I've slightly hinted that order isn't important in the first example commit title :)
|
@prah23 Should I leave a commit message content follow-up to you? |
Link to Commit Style (GitLint) sections updated.
neiljp
force-pushed
the
2021-06-18-gitlint
branch
from
80f87a3 to
d251810
Compare
|
Updated per feedback. Hope to merge in the next <24h. |
|
Sure, I'll write it and open another PR. |
|
OK, let's get this merged - feel free to discuss and update while I'm away so we can help clarify anything else people have noticed 👍 |
Link to Commit Style (GitLint) sections updated.