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

Do not strip excess leading dashes on long option names #21

Merged
merged 3 commits into from
Dec 21, 2021

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Dec 4, 2021

Problem

All leading dashes on options are being stripped. This was to allow -foo treated the same as --foo for some circumstances, but that approach is being reconsidered to fully support short option flags and have simple consistent behaviour.

Note: the preflight checks in current limited code mean this is only affecting two or more dashes.

See #7 and #2

Solution

Only strip the two leading dashes of an option with two or more leading dashes.

---foo is parsed as an option named -foo.

(I followed the lint rules for the test file, rather than the local code style. I did not modify the rest of the file to pass the linter to keep this PR focused.)

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.

@joesepi personally, I think this is a good resolution to #7, if you provide an extra dash you get the variable -foo, this is nice because the person using the library can then choose how they deal with this value.

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 think the tests will need to be updated to flags.

@shadowspawn
Copy link
Collaborator Author

Test updated.

@bcoe bcoe merged commit f848590 into pkgjs:main Dec 21, 2021
@bcoe
Copy link
Collaborator

bcoe commented Dec 21, 2021

Asking for forgiveness rather than permission, as this was open for a couple weeks 😆

I like this simplfication to the parser.

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.

None yet

2 participants