Fixes error on | character in CLI
#1634
Conversation
|
@publiclab/is-reviewers @jywarren please review this! |
Codecov Report
@@ Coverage Diff @@
## main #1634 +/- ##
==========================================
- Coverage 66.67% 64.88% -1.80%
==========================================
Files 130 135 +5
Lines 2686 2828 +142
Branches 433 458 +25
==========================================
+ Hits 1791 1835 +44
- Misses 895 993 +98
|
|
Can you explain, in brief, what caused this and how you fixed it? Thanks. |
|
@harshkhandeparkar The error was because the syntax for taking input in CLI was So I rewrote how the inputs were taken to support the following syntax Similar to how we take inputs from URL hash. |
|
Oh so the CLI and URL input formats werr different. |
| input + | ||
| ': ' + | ||
| options[input].desc | ||
| ); | ||
| }); | ||
|
|
||
| if (program.config) { |
There was a problem hiding this comment.
What was this and why was it removed?
There was a problem hiding this comment.
This was used to get input for options for different modules using -c flag, I removed this to support new format.
There was a problem hiding this comment.
Now we can't directly give input? Or is there a different flag for it now?
There was a problem hiding this comment.
Ah just need clarification on this - thanks!
There was a problem hiding this comment.
Before:
./index.js -s "webgl-distort" -c '{"nw":"100,100","se":"40,40"}' -i examples/images/test.png
Now:
./index.js -s "webgl-distort{nw:100%2C100|se:40%2C40}" -i examples/images/test.png
So since we used to give options seperately therefore we needed the -c flag but now the option values are given inside the same string(just like the url) so it isn't needed anymore.
| program.steps = sequencer.stringToJSON(program.step); | ||
|
|
||
| var stepNames = []; | ||
| program.step.split(',').forEach(v => stepNames.push(v.split('{')[0])); |
There was a problem hiding this comment.
You could use array map function.
stepNames = program.step.split(',').map(....).
Let's make the code concise and modern.
There was a problem hiding this comment.
Yea you're right, I will do that ASAP!
|
|
|
Hi all! Just coming back to this, thank you so much! I hope you're all well. I'm going to update this but I wonder if folks could help point out where CLI is tested out... i can't seem to find it and I just reorganized the test runners so I'd like to have a separate job for them alongside the others. Then fixes like this will also be easier to review and check tests for!
|
|
Just an update - now we can test this out on GitPod too, when you update the branch! We can actually open this now, above: https://gitpod.io/#https://github.com/publiclab/image-sequencer/pull/1634 Do you think you could add a basic CLI test? Perhaps we actually need a simple test file in a shell script. I'm going to try. |
|
Noting that we should also link this to #1694 ! |
|
Shall we merge this after #1718, the CLI tests? Thanks! |
|
Noting CLI tests now exist:
|
| input + | ||
| ': ' + | ||
| options[input].desc | ||
| ); | ||
| }); | ||
|
|
||
| if (program.config) { |
There was a problem hiding this comment.
Ah just need clarification on this - thanks!
|
Hi all, just wondering what the status of this is now -- thank you!!! |
harshkhandeparkar
added
the
almost-complete
harshkhandeparkar
removed
the
almost-complete
Fixes #1033
Fixes #1040
This PR fixes error on
|character in CLI inputsThe supported syntax after this PR would be
./index.js -s "contrast{contrast:55},blur{blur:2.4}" -i examples/images/test.pngMake sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm run test-all@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!