-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor!: rework results to remove redundant flags
property and store value true for boolean options
#83
refactor!: rework results to remove redundant flags
property and store value true for boolean options
#83
Conversation
- Does `--no-foo` coerce to `--foo=false`? For all options? Only boolean options? | ||
- no, it sets `{values:{'no-foo': true}}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated to this PR, but it would be nice to have parseArgs handle "what if i do --foo --no-foo
" for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note that's something you could trivially build on top of parseArgs
if it reports indexes of arguments (#84).
Co-authored-by: Jordan Harband <[email protected]>
…ub.com:shadowspawn/parseargs into feature/refactor-results-leaner-meaner-cleaner
Got a couple of 👍 (thanks) but still looking for an approving review before can merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this simplification, if we've thought things through and don't think it will limit any use-cases.
I worked through some core use cases in #38 using various patterns for the results, and didn't find any holes. (At the time the approach used in this PR was just included for interest!) |
@shadowspawn I think this is ready to go IMO, except it appears to have one failing test. |
I'll need to add |
👍 if you're feeling good about the PR, I can do my best to merge early this week and get our current state synced with the Node.js PR. |
Ready, barring more changes landing on main first. 😄 |
Leaner, meaner, cleaner!
See #70 for longer description (deleting
flags
in #70 (comment)). See #80 for previous proposal to renameflags
rather than delete, and this PR built on that.The big change in this PR is removing the
flags
property in the results.flags
is not encoding any additional information, and is not adding much. The lack of a compelling use case has made renaming it difficult!In parse results:
flags
true
invalues
for boolean options (rather thanundefined
)In README, a number of minor fixes after reading through examples carefully:
--
description to match current implementation