Skip to content

Add clarifications about passThroughOptions handling of unknown options to docs and comments#1942

Closed
aweebit wants to merge 3 commits intotj:developfrom
aweebit:feature/passThroughOptions-unknown-option
Closed

Add clarifications about passThroughOptions handling of unknown options to docs and comments#1942
aweebit wants to merge 3 commits intotj:developfrom
aweebit:feature/passThroughOptions-unknown-option

Conversation

@aweebit
Copy link
Copy Markdown
Contributor

@aweebit aweebit commented Aug 5, 2023

Problem

#1936

Solution

What caused my confusion turned out to be desired behavior.

This PR includes clarifications that could help avoid confusion in the future.

ChangeLog

Fixed

  • extended documentation about .passThroughOptions() by a missing mention of unknown option handling

aweebit added 2 commits August 5, 2023 18:14
@aweebit
Improve comment on passThroughOptions
f07ba2c 
Fixes tj#1936 partially (docs need an update, too)

(cherry picked from commit b11e940)
@aweebit
Add clarification about passThroughOptions to docs
7bad142
This was referenced Aug 5, 2023
Closed
Merged
@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Aug 7, 2023

I suspect this was driven in part by confusion with interaction with .allowUnknownOption() which is a second-class setting.

On balance, I don't think the comment in the README is helpful for the majority of readers, and the comment in the code was unclear to me and I know what the code does. Given how many other issues and PRs you have open at the moment, I am just closing this one.

Related comments about .allowUnknownOption() :

@shadowspawn shadowspawn closed this Aug 7, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
@aweebit
Refactor parseOptions()
f8e5faf 
- Store onlyKnownOptionsSoFar value instead of computing
- Removed passThroughOptions comment considered unclear (see tj#1942)

(cherry picked from commit d7e2399)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aweebit @shadowspawn