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

fix: dot-notation now respects strip-dashed #288

Conversation

Mike111177
Copy link
Contributor

@Mike111177 Mike111177 commented Jun 26, 2020

Fixes #223

@Mike111177
Copy link
Contributor Author

Actually, just realized, this may infinitely a circular reference into a coerced arg.... fix pending....

@Mike111177 Mike111177 marked this pull request as draft June 27, 2020 16:11
@Mike111177 Mike111177 force-pushed the fix_dot-notation_doesn't_respect_strip-dashed branch from 6982e63 to 759aba7 Compare June 27, 2020 17:34
@Mike111177
Copy link
Contributor Author

Fixed, ready

@Mike111177 Mike111177 marked this pull request as ready for review June 27, 2020 17:35
@Mike111177 Mike111177 force-pushed the fix_dot-notation_doesn't_respect_strip-dashed branch from 759aba7 to 5a2f77e Compare June 27, 2020 17:55
@bcoe
Copy link
Member

bcoe commented Jul 18, 2020

Hey @Mike111177 sorry for the slow reply. Bother you to rebase against the main branch, I've been working on landing some of our TypeScript work.

@bcoe
Copy link
Member

bcoe commented Aug 11, 2020

@Mike111177 just a friendly note, to ask whether this is still something you'd like to see landed.

@Mike111177 Mike111177 force-pushed the fix_dot-notation_doesn't_respect_strip-dashed branch from 5a2f77e to 8f40f1f Compare August 11, 2020 21:38
@Mike111177
Copy link
Contributor Author

Completely forgot about it lol, here you go.

@Mike111177 Mike111177 force-pushed the fix_dot-notation_doesn't_respect_strip-dashed branch from 8f40f1f to e9dd129 Compare August 11, 2020 21:54
@bcoe
Copy link
Member

bcoe commented Aug 27, 2020

@Mike111177 I owe you review, haven't forgotten about this myself.

Copy link
Member

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

This looks like a good fix to me 👍 could I bother you to rebase and fix the one nit?

lib/yargs-parser.ts Outdated Show resolved Hide resolved
test/yargs-parser.cjs Outdated Show resolved Hide resolved
test/yargs-parser.cjs Outdated Show resolved Hide resolved
@bcoe
Copy link
Member

bcoe commented Sep 20, 2020

@@Mike111177 shoot, with my rebase I seem to now be in a position where tests aren't passing.

Copy link
Member

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

Sorry for being so slow with review on this issue, I've been a little bit swamped.

The reason I didn't land immediately, is that I'm a little concerned about adding so much logic related to handling a single config variation for dot-notation.

Could we figure out a way to pull the dot-notation check into a helper function perhaps that we could reuse to make dot notation respect configuration in a more general sense? I feel like there are many config settings that dot notation doesn't respect.

@bcoe
Copy link
Member

bcoe commented Feb 15, 2021

@Mike111177 just checking if you still want to land this.

@bcoe bcoe closed this Mar 5, 2021
@bcoe
Copy link
Member

bcoe commented Mar 5, 2021

@Mike111177 let me know when/if you would like to dust this off 👍

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.

dot-notation doesn't respect strip-dashed
2 participants