feat(version): Ability to disable commit hooks on versioning #5640
Conversation
src/cli/commands/version.js
Outdated
| @@ -130,6 +131,14 @@ export async function setVersion( | |||
| return () => Promise.resolve(); | |||
| } | |||
|
|
|||
| function buildCommitArgs(args): Array<string> { | |||
There was a problem hiding this comment.
I don't think abstracting this into a separate function adds much value unless you use it in a unit test.
src/cli/commands/version.js
Outdated
| @@ -130,6 +131,14 @@ export async function setVersion( | |||
| return () => Promise.resolve(); | |||
| } | |||
|
|
|||
| function buildCommitArgs(args): Array<string> { | |||
| args = args || ['commit']; | |||
There was a problem hiding this comment.
Don't think allowing an empty args value is useful. It adds a new code path to be tested and currently this code path is not even being used. I'd rather us to enforce an array argument through proper flow definitions.
src/cli/commands/version.js
Outdated
| @@ -130,6 +131,14 @@ export async function setVersion( | |||
| return () => Promise.resolve(); | |||
| } | |||
|
|
|||
| function buildCommitArgs(args): Array<string> { | |||
| args = args || ['commit']; | |||
| if (!flags.commitHooks || !config.getOption('version-commit-hooks')) { | |||
There was a problem hiding this comment.
I think this part would be more useful as a separate function: isCommitHooksDisabled(). Then you'd use it like as follows:
// create git commit
await spawnGit(['commit', ...(isCommitHooksDisabled() ? ['-n'] : []), '-m', message], {cwd: gitRoot});
faazshift
force-pushed
the
commit-hook-control
branch
from
d388277
to
c965f30
Compare
|
@BYK Thanks for the feedback. I've made the requested changes and added test cases. |
faazshift
force-pushed
the
commit-hook-control
branch
3 times, most recently
from
7846b8e
to
2ec7d4d
Compare
faazshift
force-pushed
the
commit-hook-control
branch
from
2ec7d4d
to
633c7fa
Compare
|
|
||
| test('run version and make sure git commit hooks are enabled by default', async (): Promise<void> => { | ||
| const fixture = 'no-args'; | ||
| await fs.mkdirp(path.join(fixturesLoc, fixture, '.git')); |
| await fs.mkdirp(path.join(fixturesLoc, fixture, '.git')); | ||
|
|
||
| return runRun([], {newVersion, gitTagVersion}, fixture, (): ?Promise<void> => { | ||
| const gitArgs = ['commit', '-m', 'v2.0.0', '-n']; |
There was a problem hiding this comment.
#protip: since this looks a lot like snapshot testing, you can probably do expect(spawn.mock.calls).toMatchSnapshot('no-hooks-from-config') and similar in other tests ;)
BYK
changed the title
Summary
Last year I contributed the ability to disable commit hooks when versioning with npm (See this commit). It makes sense to add the same functionality to yarn.
The original motivation for this feature is based on a pre-commit hook that included linting and testing a large code base, taking ~20 seconds to run. When that hook is run on the preceding commits, it's unnecessary and time-taking to do so on versioning. Because of that, it's nice to have the option to disable that git hook.
Test plan
Added lower level integration tests.