Skip to content

Remove --require CLI option (completes #1069) #1070

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

Merged
merged 2 commits into from
Oct 10, 2016

Conversation

jacobkahn
Copy link
Contributor

This completes #1069 as part of #1048 - removes --require/-r from the CLI interface, corrects documentation where relevant, modifies CLI tests accordingly.

Not sure if it makes sense to add a deprecation notice or any clarifications to docs - these changes remove mentions of the flag and defer to package.json configuration guidelines.

@jacobkahn jacobkahn force-pushed the remove-require-cli-flag branch from 58923ae to f94b58d Compare October 7, 2016 20:47
@vadimdemedes
Copy link
Contributor

I'm sure many people use --require babel-register or similar, I think deprecation message pointing to configuration in package.json is a must have.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with showing a warning when --require is used. See https://github.com/jacobkahn/ava/blob/f94b58df23202749691e3e82305486efd6a9854d/cli.js#L119 for an example of how we check for --tap flags when watch mode is used.

@@ -129,7 +126,6 @@ if (
var api = new Api({
failFast: cli.flags.failFast,
serial: cli.flags.serial,
require: arrify(cli.flags.require),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, cli.flags.require would default to the config from package.json, through this line. Api still needs the require option, so this should now be require: arrify(conf.require),.

@novemberborn
Copy link
Member

And wow @jacobkahn that was quick!

@jacobkahn
Copy link
Contributor Author

Re-added that arg into API - thanks for catching that. Right now the deprecation message mentions the docs, then exits. Do we think this behavior is alright?

@novemberborn novemberborn merged commit 17119bc into avajs:master Oct 10, 2016
@novemberborn
Copy link
Member

🎊🎉 Thanks @jacobkahn!

If you're up for another challenge, #937 is fairly similar 😄

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

Successfully merging this pull request may close these issues.

4 participants