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
argv-store should convert the strings "true" and "false" to booleans #72
Comments
I agree that parsing "true" and "false" into booleans would be useful. Right now I am using optimist to load any boolean parameters but it would be simpler to just use nconf |
What if you'd like myBool (or any variable) to actually be the string "true" though? Some sort of flag at the command-line level would likely be needed, adding complexity. My suggestion would be to cast it to a bool on your end, like so:
|
I think it's more common to use true and false as booleans. It makes more sense to apply the manual cast in case of a string usage of myBool because it would happen less often than the other way round. Don't you agree? |
In theory you could relegate command line arguments to being flags proper, i.e., booleans, and have environment variables and config file settings' values still be parsed as strings, but you'd probably have to convince the author, Charlie Robbins, that argv should always be booleans, which would break backwards compatibility. It'd probably be worth surveying usage patterns, but I don't think we should assume that command line flags are always meant to be booleans; many CLI programs that are configurable via flags accept various types. Type -h or --help with any number of *nix programs to see what I mean. I'm not saying I don't sympathize with your want, but perhaps there's some sort of compromise? |
I ran into this problem as well, and had to work around this using a trick:
In this way, your app does not require any change at all |
Or nconf could be updated to use commander, which in addition to being more current and widely used than optimist, has the advantage of having typed arguments with coercion (solving this exact problem). And as a bonus, commander is also not deprecated ;) |
Any updates on this? Had to deal with this situation myself today. @BobDickinson (old) suggestion seemed reasonable at the time. I'm throwing yargs into the mix as well. EDIT: Don't mind me. I noticed nconf is already using yargs. |
Our workaround is that we create a memory store and read the env vars into it (filtering to just pick the ones we want, and converting booleans and null):
|
@BobDickinson You could consider using Also, regarding boolean values, what about using simple So, instead of EDIT: This was suggested before here: #72 (comment)
|
JSON.parse() assumes that the value being parsed is valid JSON. I have no idea what values might be in cmd line args or env vars and whether they will be valid JSON. For example, a string with a brace in the middle might be a perfectly valid config value, but will throw an exception if you call JSON.parse on it. And if the string was actually a JSON object, using the output of JSON.parse would change the behavior by parsing that as an object instead of treating it like a string (you might want that behavior, but it's definitely a different contract than the behavior I was trying to fix). My goal was to fix a specific problem without causing any unintended side-effects, and I prefer to be explicit as opposed to trying to save a couple of lines of code. YMMV. |
* simple parse, #72 * documentation for tryParse option * Combine JSON parsing and simple parsing
* simple parse, indexzero/nconf#72 * documentation for tryParse option * Combine JSON parsing and simple parsing
Example
node app.js --myBool true
results innconf.get('myBool') // 'true'
I think converting boolean-string to real booleans would be nice, because that's what you are expecting - i think the same applies for the env-store.
I will attach a pull request if you agree on my proposal.
The text was updated successfully, but these errors were encountered: