-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add in proper handling of spaces and quotes in ISVC parsing #3592
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6c3ffe4
to
4732326
Compare
This adds a rudimentary parser that should handle most cases for inferenceservice argument parsing. Note, I'm not a JS expert, so it's possible there's some degerenate edge case with this, but in testing it worked fine
2163428
to
42b88b2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3592 +/- ##
==========================================
+ Coverage 85.16% 85.18% +0.01%
==========================================
Files 1392 1392
Lines 31856 31868 +12
Branches 8925 8927 +2
==========================================
+ Hits 27130 27146 +16
+ Misses 4726 4722 -4
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
const regex = /(?:[^\s"']+|"[^"]*"|'[^']*')+/g; | ||
let match: RegExpExecArray | null; | ||
|
||
while ((match = regex.exec(input)) !== null) { |
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.
Looking closer at this - what iterates this loop? does regex.exec(input)
with the same input multiple times step to the next match? A quick glance at docs says yes - interesting, I haven't used it that way before.
I wonder if it would be simpler to use regex.match
instead of regex.exec
to just get back an array of all the matches, and then do a forEach
or reduce
on those? I'm nitpicking though, it's probably fine as-is.
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.
I'm fine with changing solution, that's just the one that it gave me
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.
If it works it works, but I may tinker with an alternative when I have more time to review. Did you still plan on adding / have time to add unit tests here?
https://issues.redhat.com/browse/RHOAIENG-16900
Description
This adds a rudimentary parser that should handle most cases for inferenceservice argument parsing.
Note, I'm not a JS expert, and wrote it with the help of my internal AI, so it's possible there's some degerenate edge case with this, but in testing it worked fine
How Has This Been Tested?
Code used to test:
parseCommandLine.ts.txt
(Note it misbehaves a bit on some of the negative tests, but the normal test cases it works fine on)
Test Impact
Tested with the above tests, caveats noted
Request review criteria:
Self checklist (all need to be checked):
After the PR is posted & before it merges:
main