Skip to content

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

Merged
bcoe merged 16 commits into
pkgjs:mainfrom
shadowspawn:feature/refactor-results-leaner-meaner-cleaner
Apr 11, 2022
Merged

refactor!: rework results to remove redundant flags property and store value true for boolean options#83
bcoe merged 16 commits into
pkgjs:mainfrom
shadowspawn:feature/refactor-results-leaner-meaner-cleaner

Conversation

@shadowspawn

@shadowspawnshadowspawn commented Mar 19, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@shadowspawnshadowspawn mentioned this pull request Mar 19, 2022
Closed
ljharb
ljharb reviewed Mar 19, 2022
View reviewed changes
Comment threadREADME.md Outdated
Comment threadREADME.md
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}}`

@ljharbljharbMar 19, 2022

Copy link
Copy Markdown
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

@bakkotbakkotMar 19, 2022

Copy link
Copy Markdown
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).

Comment threadREADME.md Outdated
shadowspawnand others added 3 commits March 19, 2022 17:38
@shadowspawn@ljharb
Expand description
167f9e7
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@shadowspawn
Improve --foo=true description
9defcc3
@shadowspawn
Merge branch 'feature/refactor-results-leaner-meaner-cleaner' of gith…
2b56411
…ub.com:shadowspawn/parseargs into feature/refactor-results-leaner-meaner-cleaner
@shadowspawnshadowspawn added this to the Merge into Node.js milestone Mar 20, 2022
@shadowspawn

shadowspawn commented Mar 30, 2022

Copy link
Copy Markdown
CollaboratorAuthor

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

@shadowspawnshadowspawn mentioned this pull request Mar 30, 2022
Closed
aaronccasanova
aaronccasanova approved these changes Apr 8, 2022
View reviewed changes

@aaronccasanovaaaronccasanova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@shadowspawnshadowspawn mentioned this pull request Apr 9, 2022
Closed
@bcoebcoe mentioned this pull request Apr 9, 2022
Merged
4 tasks
bcoe
bcoe approved these changes Apr 9, 2022
View reviewed changes

@bcoebcoe left a comment

Copy link
Copy Markdown
Collaborator

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

shadowspawn commented Apr 9, 2022

Copy link
Copy Markdown
CollaboratorAuthor

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 commented Apr 10, 2022

Copy link
Copy Markdown
Collaborator

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

@shadowspawn

shadowspawn commented Apr 11, 2022

Copy link
Copy Markdown
CollaboratorAuthor

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

@bcoe

bcoe commented Apr 11, 2022

Copy link
Copy Markdown
Collaborator

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

shadowspawn commented Apr 11, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Ready, barring more changes landing on main first. 😄

@bcoe
bcoe merged commit be153db into pkgjs:mainApr 11, 2022
@github-actionsgithub-actionsBot mentioned this pull request Apr 11, 2022
Merged
@shadowspawn
shadowspawn deleted the feature/refactor-results-leaner-meaner-cleaner branch June 5, 2022 03:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments
@bakkotbakkotbakkot left review comments
@bcoebcoebcoe approved these changes
@aaronccasanovaaaronccasanovaaaronccasanova approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Merge into Node.js

Development

Successfully merging this pull request may close these issues.

5 participants

@shadowspawn@bcoe@ljharb@bakkot@aaronccasanova