-
Notifications
You must be signed in to change notification settings - Fork 121
fix: correctly parse unknown args with dashes when they resemble known args #502
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
Conversation
|
(Thanks for the notes on the hacks to run the tests. I did a draft of updates in #471 but have not looked at it since.) |
|
@shadowspawn I'll leave this PR to you, since it looks like it needs to be sequenced with your refactoring work? |
|
Oops, I forgot about this one. Thanks @Xunnamius , I will take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The point of the regexp is a bit opaque to me, but based on the name of the variable and checking the behaviour in a regular expression tester, I think this make sense.
The new tests fail with the old regular expression and pass with the new regular expression.
|
(I am having another look at |
|
@shadowspawn perhaps we could add a performance test, like these, https://github.com/yargs/yargs-parser/blob/main/test/yargs-parser.mjs#L4005 that hits this regex specifically on a branch. We don't need to fix the performance problem in this PR, but it's good to know if this PR incidentally addresses it. |
shadowspawn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undoing my approval as have a new PR candidate in #508
Resolves #501
Additionally, to compile yargs-parser and pass the CJS unit tests, I had to make two changes I did not include in this PR:
typescript@5and add--noCheckto the "compile" and "pretest" scripts inpackage.jsonto work around some benign? unrelated type issues elsewhere in the codebase:"scripts": { ... - "pretest": "rimraf build && tsc -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs", + "pretest": "rimraf build && tsc --noCheck -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs", ... - "compile": "tsc", + "compile": "tsc --noCheck", ... }, ... "devDependencies": { ... - "typescript": "^4.0.0" + "typescript": "^5.8.2" },transformDefaultExport-related configuration fromrollup.config.jssince the package seems to be non-functional; I don't believe the transform is required by non-EOL JS runtimes anyway: