-
Notifications
You must be signed in to change notification settings - Fork 914
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
Support linting from the last tag #4110
Support linting from the last tag #4110
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
0116609
to
f60ef02
Compare
f60ef02
to
8fd2f67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some commentary I hope helps with the review.
const gitCommandResult = await execa( | ||
'git', | ||
['log', '-1', '--pretty=format:%B'], | ||
{cwd} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find a bug, fix a bug. Previously using the --last
option didn't observe --cwd
.
test('should only read the last commit', async () => { | ||
const cwd: string = await git.bootstrap(); | ||
|
||
await execa('git', ['commit', '--allow-empty', '-m', 'commit Z'], {cwd}); | ||
await execa('git', ['commit', '--allow-empty', '-m', 'commit Y'], {cwd}); | ||
await execa('git', ['commit', '--allow-empty', '-m', 'commit X'], {cwd}); | ||
|
||
const result = await read({cwd, last: true}); | ||
|
||
expect(result).toEqual(['commit X']); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locked in the --last
fix with a test. Previously it was not covered.
'git', | ||
[ | ||
'describe', | ||
'--abbrev=40', | ||
'--always', | ||
'--first-parent', | ||
'--long', | ||
'--tags', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These git arguments have been battle tested against dozens of repos over several years. Most git output is designed for humans, not for machines (i.e. deserialization), so it takes some work to get stable, deterministic output.
// Description will be in the format: <tag>-<count>-g<hash> | ||
// Example: v3.2.0-11-g9057371a52adaae5180d93fe4d0bb808d874b9fb | ||
// Minus zero based (1), dash (1), "g" prefix (1), hash (40) = -43 | ||
const tagSlice = stdout.lastIndexOf('-', stdout.length - 43); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a few ways to skin this cat. The complexity comes from the fact that tags can themselves contain the dash -
character (i.e. v1.2.3-alpha.0
), and git doesn't escape anything. Need to work backwards from the last dash we can be sure was produced by git.
8fd2f67
to
afa7823
Compare
Thanks for the comments! |
Once this is rebased it can be merged and a release can be done. Might take a couple of days. |
afa7823
to
8898f5a
Compare
Description
Resolves #2507, which has the full problem statement. Also fixed a bug with the
--last
option.In draft mode to get the discussion going. Will open it up for real after more thorough testing.Testing complete (see below) and this is now ready to review.Motivation and Context
See #2507.
Note that if there are no tags, such as in a brand new repo, commitlint will always pass. The idea is that it should be possible to add
commitlint --from-last-tag
to CI at the very beginning of a repo and never remove it. There is no need to document special steps, like adding an initial tag.Usage examples
How Has This Been Tested?
Unit tests and locally against this repo:
Output
Types of changes
Checklist: