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

refactor!: rework results to remove redundant flags property and store value true for boolean options #83

Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Mar 19, 2022

Leaner, meaner, cleaner!

See #70 for longer description (deleting flags in #70 (comment)). See #80 for previous proposal to rename flags 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:

  • remove flags
  • store true in values for boolean options (rather than undefined)

In README, a number of minor fixes after reading through examples carefully:

  • update result property descriptions
  • update -- description to match current implementation
  • fix one example that got broken in a previous refactor
  • replace stale "withValue" with "type:string"
  • replace "flag" word with "option" or " boolean option" as appropriate
  • other small tweaks to wording

README.md Outdated Show resolved Hide resolved
Comment on lines +187 to +188
- Does `--no-foo` coerce to `--foo=false`? For all options? Only boolean options?
- no, it sets `{values:{'no-foo': true}}`
Copy link
Member

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

Copy link
Collaborator

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).

README.md Outdated Show resolved Hide resolved
shadowspawn and others added 3 commits March 19, 2022 17:38
Co-authored-by: Jordan Harband <[email protected]>
…ub.com:shadowspawn/parseargs into feature/refactor-results-leaner-meaner-cleaner
@shadowspawn shadowspawn added this to the Merge into Node.js milestone Mar 20, 2022
@shadowspawn
Copy link
Collaborator Author

Got a couple of 👍 (thanks) but still looking for an approving review before can merge.

Copy link
Collaborator

@aaronccasanova aaronccasanova left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@bcoe bcoe left a 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.

@shadowspawn
Copy link
Collaborator Author

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!)

@bcoe bcoe removed the do-not-merge label Apr 10, 2022
@bcoe
Copy link
Collaborator

bcoe commented Apr 10, 2022

@shadowspawn I think this is ready to go IMO, except it appears to have one failing test.

@shadowspawn
Copy link
Collaborator Author

I'll need to add type:* in various tests after the merge.

@bcoe
Copy link
Collaborator

bcoe commented Apr 11, 2022

I'll need to add type:* in various tests after the merge.

👍 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.

@shadowspawn
Copy link
Collaborator Author

Ready, barring more changes landing on main first. 😄

@bcoe bcoe merged commit be153db into pkgjs:main Apr 11, 2022
@shadowspawn shadowspawn deleted the feature/refactor-results-leaner-meaner-cleaner branch June 5, 2022 03:15
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.

5 participants