Add in proper handling of spaces and quotes in ISVC parsing #3592
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 |
Xaenalt
force-pushed
the
dashboard-argparse-fix
branch
2 times, most recently
from
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
Xaenalt
force-pushed
the
dashboard-argparse-fix
branch
from
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.
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.
I'm fine with changing solution, that's just the one that it gave me
There was a problem hiding this comment.
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