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

Passes flags to commands #230

Merged
merged 16 commits into from
Oct 15, 2016
Merged

Passes flags to commands #230

merged 16 commits into from
Oct 15, 2016

Conversation

ccpricenytimes
Copy link
Contributor

Fixes #150
Fixes #113

@@ -51,7 +51,7 @@ module.exports = (config) => {
serverCompiler.options.output.path, `${Object.keys(serverCompiler.options.entry)[0]}.js`
);

nodemon({ script: serverPath, watch: [serverPath] })
nodemon({ script: serverPath, watch: [serverPath], nodeArgs: [...flags] })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just be nodeArgs: flags

const config = kytConfigFn(optionalConfig);
action(config, program);
action(config, program, flags);
Copy link
Contributor

@tizmagik tizmagik Oct 8, 2016

Choose a reason for hiding this comment

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

It looks like program isn't actually used be any of the actions, and instead just need flags. Can you double check that, and if true, maybe we can simplify this signature to just this:

action(config, flags);

@ccpricenytimes
Copy link
Contributor Author

@tizmagik Thanks for the feedback. Made the updates. setup is using program but I changed the order to make it a little cleaner

@tizmagik
Copy link
Contributor

tizmagik commented Oct 9, 2016

Spent some time with this PR, still some issues I see:

This doesn't fully fix #150 e.g. we still can't do this:

kyt lint -- --fix

In that same vein, there is no distinction between program.args and flags. Ideally, we should be able to split the CLI flags on --, so that the following:

kyt cmd --arg1 --arg2 -- --flag1 --flag2

Produces:

args = ['arg1', 'arg2']; // we can probably drop "program" since that's an implementation detail
flags = ['flag1', 'flag2'];

If you want, we can say this fixes #113 but leave #150 open for a follow-up PR.

@delambo
Copy link
Member

delambo commented Oct 10, 2016

I actually need this functionality right now for a starter kyt I'm building. I haven't fully formed a solution, but I think we could drop the eslint Node API and use the eslint cli. For example:

shell.exec(`${eslint} -c ${configFile} --ext .js ${USER_FLAGS} ${srcPath}`);

@ccpricenytimes
Copy link
Contributor Author

Hey @tizmagik,

Ok more updates! Lint now takes any flags. I also separated flags and args. The way args works through commander is that they get parsed and placed into the command object, so I'm passing along that object to be used by setup, and creating the array of flags from anything that's a string.

Let me know if you see anything else. Thanks!

@delambo test this out with your starter-kyt and let me know if it works


if (report.errorCount === 0) {
logger.end(`Your JS looks ${report.warningCount === 0 ? 'great ✨' :
const cmd = `eslint src/ -c ${configFile} --color --env browser ${flags.join(' ')}`;
Copy link
Contributor

@tizmagik tizmagik Oct 12, 2016

Choose a reason for hiding this comment

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

Could this just be eslint src/ -c ${configFile} ${flags.join(' ')}? E.g. drop the --color and --env flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to specify color and browser since we're running it with shelljs now

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to put env: { browser: true} in the base eslint config instead, this way it's overridable at the command-line by the user.

I believe color is enabled by default anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved env but color doesn't work without the flag.

kyt dev -- --inspect
```
will run the [node debugging for Chrome DevTools](https://medium.com/@paul_irish/debugging-node-js-nightlies-with-chrome-devtools-7c4a1b95ae27#.mpuwgy17v)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also mention for the test and lint commands? Or, mention that flags can now be passed generically in almost all cases (except for lint-style) so it doesn't read as necessarily specific to these two commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added separate notes. good catch!

Copy link
Contributor

@tizmagik tizmagik left a comment

Choose a reason for hiding this comment

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

A couple minor things otherwise LGTM. Nice job!

@delambo delambo merged commit 11252a0 into master Oct 15, 2016
@delambo delambo deleted the passes-flags-to-commands branch October 15, 2016 23:49
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.

3 participants