feat: understands --config <FILE> option in conventionalChangelogArgs
#927
Conversation
aseure
force-pushed
the
feat/enable-config-option
branch
3 times, most recently
from
6de01f3
to
fdaa4df
Compare
aseure
force-pushed
the
feat/enable-config-option
branch
from
fdaa4df
to
2c57b65
Compare
packages/shipjs/src/step/prepare/__tests__/updateChangelog.spec.js
Outdated
Show resolved
Hide resolved
…c.js Co-authored-by: Haroen Viaene <hello@haroen.me>
|
|
||
| describe('prepareParams', () => { | ||
| it('loads configuration from --config option', () => { | ||
| parseArgs.mockImplementation(jest.requireActual('../../../util').parseArgs); |
There was a problem hiding this comment.
I had to restore the original implementation of parseArgs because I made a wrong choice in the beginning of Ship.js to globally mock many things including util. It's so hidden and hard to find out things like this :(
There was a problem hiding this comment.
Yes something was wrong with the parseArgs but I just discovered this morning that you finished the PR.
Just to understand: what does this test mock exactly? It seems we are mocking parseArgs but at the end, the config seems correctly generated, how does that work?
There was a problem hiding this comment.
Actually parseArgs was not supposed to be mocked, but I mocked everything under util globally, which made parseArgs returned nothing. So it made the test case failed. This line parseArgs.mockImplementation(jest.requireActual('../../../util').parseArgs); is restoring its original implementation.
There was a problem hiding this comment.
Got it, that's why it was returning me undefined and I couldn't see the console.log() I've added in its implementation! Thanks for letting me know. (my 2 cents on that, you should probably do it the other way around when testing: avoid to use global mocking, and use it explicitely when you need it in your tests).
| const configPath = tempWrite.sync(configString); | ||
| const configDir = path.basename(path.dirname(configPath)); | ||
|
|
||
| const { args } = prepareParams({ | ||
| dir: configDir, | ||
| conventionalChangelogArgs: `--config ${configPath}`, | ||
| conventionalChangelogArgs: `-i CHANGELOG.md -s --config ${configPath}`, |
There was a problem hiding this comment.
I noticed you added the -i/--infile option so that there is not the undefined error down the line with this flag. Too late for this PR as you already closed it, but maybe things could be improved here: arguments parsing shouldn't yield an error if a flag is missing, no?
There was a problem hiding this comment.
You're right.
shipjs/packages/shipjs/src/step/prepare/updateChangelog.js
Lines 100 to 101 in 06c7226
We have these two lines expecting to have infile and outfile inside the parsed args. So the correct way to fix it would be to throw a better error message to dev and to document that these are mandatory.
No description provided.