Skip to content

Separate config keys from CLI flags #1046

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

Closed
novemberborn opened this issue Sep 21, 2016 · 19 comments
Closed

Separate config keys from CLI flags #1046

novemberborn opened this issue Sep 21, 2016 · 19 comments

Comments

@novemberborn
Copy link
Member

Currently the config object in the package.json is passed as-is to meow, providing default values for the CLI flags. This means that config keys need to match the CLI flags. CLI flags are singular, but sometimes config keys are better when pluralized. E.g. sources versus source, and the suggested extension flag in #631 versus extensions.

@avajs/core how about we change some of the config keys to be plurals and map them to meow's default in cli.js?

@sindresorhus
Copy link
Member

Sounds good.

This would also be a good opportunity to document the package.json options separately instead of just referring to the CLI flags and show a JSON config dump.

@omerbn
Copy link

omerbn commented Nov 24, 2016

Hey guys,
I'd like to do it but need further depiction.

@novemberborn
Copy link
Member Author

@omerbn AVA loads config from package.json here:

ava/lib/cli.js

Line 24 in 262911f

var conf = pkgConf.sync('ava');

It is passed to meow here:

ava/lib/cli.js

Line 74 in 262911f

default: conf,

The goal is to pass a different object to meow where the keys match the long option names in the meow configuration, so that we can change the package.json config to use different key names. E.g. pass source to meow but use sources in package.json.

@yatharthx
Copy link
Contributor

@novemberborn what keys in particular are to be mapped in configuration object? Like the keys we want to be used as plurals.

@novemberborn
Copy link
Member Author

Hi @yatharthk. There've been a few developments since I started this issue. We won't have an extensions flag, the require flag has been removed, and we're looking to remove the source flag (#937).

I think a good place to start would be to stop passing default: conf in the call to meow. We could then merge the resulting cli object with conf. Hopefully that leads to the same behavior. It'll then allow us to decouple the config keys from the CLI flags where needed (though I don't think there are any such cases at the moment).

@yatharthx
Copy link
Contributor

@novemberborn Thanks. Shall I pick up the task of pulling out the default: conf when calling meow? Is there a relevant issue or we need to create one?

@novemberborn
Copy link
Member Author

Hey @yatharthk,

Is there a relevant issue or we need to create one?

This would be the one.

Shall I pick up the task of pulling out the default: conf when calling meow?

Thanks for volunteering!

Could you check in with @ThomasBem first? He's running into an issue for which this would be the solution, though there's a workaround available to him as well. See discussion in #1198 (comment).

@yatharthx
Copy link
Contributor

@novemberborn Thanks for pointing that out for me. 👍

@novemberborn
Copy link
Member Author

@yatharthk looks like @ThomasBem is working around the issue. Do you want to take this then?

@yatharthx
Copy link
Contributor

@novemberborn Would love to. Can you please guide me with proceeding on this? Thanks.

@novemberborn
Copy link
Member Author

@yatharthk sure, feel free to post your questions here.

@yatharthx
Copy link
Contributor

@ThomasBem I'm planning to pick this issue for removing the default: conf from call to meow. Would that be fine or you are doing that?
Also, we aren't going to deal with any cli flags here, for now; if I'm not wrong. Please correct my knowledge if I'm wrong over here. @novemberborn

@novemberborn
Copy link
Member Author

You're correct, and go ahead.

@yatharthx
Copy link
Contributor

@novemberborn I'm working on this now. Would be great if you could hint me on the right approach to merge cli object with conf. What I can understand is default creates flags on the cli object. Is it something like we merge conf into cli.flags or something? I tried this but I don't think this is something we need to do!

@novemberborn
Copy link
Member Author

I think pick the correct default values out of conf, to pass to meow. Then read either from cli.flags or conf when passing values to the Api constructor etc.

@yatharthx
Copy link
Contributor

@novemberborn So here's what I think: Instead of passing conf to meow as default, let's keep it separate and override it's properties with those of cli.flags, maybe something like Object.assign(conf, cli.flags) and then we can use conf in place of cli.flags everywhere. If this makes sense, I'll go ahead and make the changes.

@novemberborn
Copy link
Member Author

You still need to provide default values based on conf to meow. I'm OK with copying the resulting cli.flags back into conf.

@yatharthx
Copy link
Contributor

Okay I got your point here. The thing that's bit tricky for me here is understanding what you talk about picking default values out of conf. As I understand, it already contains all the default values. Shouldn't we be picking the entire conf? How do I approach the picking of defaults from conf? 🤔

@novemberborn
Copy link
Member Author

The thing that's bit tricky for me here is understanding what you talk about picking default values out of conf.

Ah! I mean when providing default to meow, we should only pass those values that meow uses. So in

ava/lib/cli.js

Line 73 in b886c5b

default: conf,
change to default: { failFast: conf.failFast, serial: conf.serial } and so forth.

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

No branches or pull requests

4 participants