Skip to content
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

feat: understands `--config <FILE>` option in `conventionalChangelogArgs` #927

Merged
merged 3 commits into from Sep 25, 2020

Conversation

@aseure
Copy link
Contributor

@aseure aseure commented Sep 23, 2020

No description provided.

@aseure aseure force-pushed the feat/enable-config-option branch 3 times, most recently from 6de01f3 to fdaa4df Sep 23, 2020
@aseure aseure force-pushed the feat/enable-config-option branch from fdaa4df to 2c57b65 Sep 23, 2020
eunjae-lee and others added 2 commits Sep 25, 2020
…c.js

Co-authored-by: Haroen Viaene <hello@haroen.me>
shipjs

describe('prepareParams', () => {
it('loads configuration from --config option', () => {
parseArgs.mockImplementation(jest.requireActual('../../../util').parseArgs);

This comment has been minimized.

@eunjae-lee

eunjae-lee Sep 25, 2020
Collaborator

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 :(

This comment has been minimized.

@aseure

aseure Sep 28, 2020
Author Contributor

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?

This comment has been minimized.

@eunjae-lee

eunjae-lee Sep 28, 2020
Collaborator

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.

This comment has been minimized.

@aseure

aseure Sep 28, 2020
Author Contributor

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).

@eunjae-lee eunjae-lee merged commit 06c7226 into main Sep 25, 2020
2 checks passed
2 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@eunjae-lee eunjae-lee deleted the feat/enable-config-option branch Sep 25, 2020
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}`,

This comment has been minimized.

@aseure

aseure Sep 28, 2020
Author Contributor

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?

This comment has been minimized.

@eunjae-lee

eunjae-lee Sep 28, 2020
Collaborator

You're right.

args.infile = path.resolve(dir, args.infile);
args.outfile = path.resolve(dir, args.outfile);

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.