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

Handle boolean flags #30

Merged
merged 2 commits into from
Nov 11, 2017
Merged

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Nov 10, 2017

Not all options require an argument, but we treated everything (minus leading shorthands) as needing one. The easiest way to demonstrate is with an example:

$ git push -f origin master

Here, -f doesn't take an argument, so returning { f: "origin" } is definitely wrong.

I'm solving this by passing a boolean array into the parser. This is so we can support both the current (by default) and the boolean syntax:

$ curl -X POST ... { x: "POST" }
$ git push -f origin master ... { _: ['push', 'origin', 'master'], f: true }

@codecov-io
Copy link

codecov-io commented Nov 10, 2017

Codecov Report

Merging #30 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #30   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          64     78   +14     
=====================================
+ Hits           64     78   +14
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a855bec...a745005. Read the comment docs.

@jorgebucaran
Copy link
Owner

@jridgewell I made some refactors to the code, mostly cosmetic and layout.

Can you rebase? 👍

@jorgebucaran jorgebucaran added the enhancement New feature or request label Nov 10, 2017
Not all options require an argument, but we treated everything (minus leading shorthands) that as needing one. The easiest way to demonstrate is with an example:

```bash
$ git push -f origin master
```

Here, `-f` doesn't take an argument, so returning `{ f: "origin" }` is definitely wrong.

I'm solving this by passing a `boolean` array into the parser. This is so we can support both the current and the boolean syntax:

```bash
$ curl -X POST ...
$ git push -f origin master
```
@jridgewell
Copy link
Contributor Author

Rebased.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Nov 10, 2017

@jridgewell Can you add these docs?

For the API section.

options.boolean

An array of options that should be parsed as booleans.

getopts(["-f", "bar"], {
  boolean: ["f"]
}) //=> { _:["bar"], f:true }

OR

An array of strings denoting options to parse as booleans.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Nov 10, 2017

@jridgewell What do you think we should do about this edge case?

const args = ["-a1"]
const opts = {
  boolean: ["a"]
}

getopts(args, opts) // => { _: [], a: '1' }

These kind of flags like -a1 are largely UNIX quirks, but perhaps users would expect the above to yield: { _: [], a: true, 1: true }.

@jridgewell
Copy link
Contributor Author

I personally don't like the short splitting, I'd rather it always treats them as individual flags. So, I don't know what the correct behavior would be.

What do the other parsers do?

@jridgewell
Copy link
Contributor Author

Alright, thinking about this some more, I think it should return { a: "1" }, just like --a=1 can return "1".

@jorgebucaran jorgebucaran merged commit 354a360 into jorgebucaran:master Nov 11, 2017
@jorgebucaran
Copy link
Owner

jorgebucaran commented Nov 11, 2017

@jridgewell Behavior is mixed and inconsistent.

Alright, thinking about this some more, I think it should return { a: "1" }, just like --a=1 can return "1".

Partially, mostly agree. We are not type casting values, which I prefer, so e.g., --foo=1, produces { foo: "1" }, and "1" is falsy truthy, so if users have: options.foo && start() their app won't break, this is good.

Still find the a1 edge case interesting, should we handle it or not? I'll think about it.

Thanks for adding this feature, it was on my todo list for a while. 🎉🎉

@jridgewell
Copy link
Contributor Author

"1" isn't falsey...

@jridgewell jridgewell deleted the booleans branch November 11, 2017 04:12
@jorgebucaran
Copy link
Owner

Truthy, I mean.

jorgebucaran pushed a commit that referenced this pull request Jan 6, 2018
Not all options require an argument, but we treated everything (minus leading shorthands) that as needing one. The easiest way to demonstrate is with an example:

```bash
$ git push -f origin master
```

Here, `-f` doesn't take an argument, so returning `{ f: "origin" }` is definitely wrong.

I'm solving this by passing a `boolean` array into the parser. This is so we can support both the current and the boolean syntax:

```bash
$ curl -X POST ...
$ git push -f origin master
```
jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018
Not all options require an argument, but we treated everything (minus leading shorthands) that as needing one. The easiest way to demonstrate is with an example:

```bash
$ git push -f origin master
```

Here, `-f` doesn't take an argument, so returning `{ f: "origin" }` is definitely wrong.

I'm solving this by passing a `boolean` array into the parser. This is so we can support both the current and the boolean syntax:

```bash
$ curl -X POST ...
$ git push -f origin master
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants