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

[patch]: include aliases when looking for defaults for booleans #32

Merged
merged 2 commits into from Apr 8, 2023

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Mar 18, 2023

The starting values for boolean options are applied before parsing, whether false or a user supplied default. The check for user supplied defaults was not including aliases. Keep it simple and do in two passes using same alias detection logic as in existing code.

Fixes: #29

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #32 (2758c33) into main (093bc85) will increase coverage by 0.03%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   98.75%   98.78%   +0.03%     
==========================================
  Files           1        1              
  Lines         161      165       +4     
  Branches       71       70       -1     
==========================================
+ Hits          159      163       +4     
  Misses          2        2              
Impacted Files Coverage Δ
index.js 98.78% <100.00%> (+0.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shadowspawn shadowspawn changed the title patch: include aliases when looking for defaults for booleans [patch]: include aliases when looking for defaults for booleans Mar 19, 2023
@shadowspawn shadowspawn marked this pull request as ready for review March 19, 2023 20:22
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This seems good, but it also seems like a bigger change than the test indicates, so I'm nervous :-)

@shadowspawn
Copy link
Collaborator Author

There is extra noise from refactoring aliasIsBoolean into isBooleanKey. I could do that refactor in a separate PR?

(I hadn't planned the refactor at the outset, but it made the changes tidier in all of #32 #33 #34.)

@ljharb
Copy link
Member

ljharb commented Apr 7, 2023

If that refactor indeed doesn't require test changes, then at least a separate commit is good :-) whether it's a separate PR or not isn't important.

@shadowspawn
Copy link
Collaborator Author

Two commits now.

(In the order that makes sense afterwards, rather than the order the work happened. 😆 )

@ljharb
Copy link
Member

ljharb commented Apr 8, 2023

(The order the work happened is always irrelevant, git has a changelog not a history :-p )

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

@ljharb ljharb merged commit 2758c33 into minimistjs:main Apr 8, 2023
574 checks passed
@shadowspawn shadowspawn deleted the fix/29 branch April 8, 2023 08:55
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.

A boolean's default value does not work when an alias is used
3 participants