image-sequencer icon indicating copy to clipboard operation
image-sequencer copied to clipboard

Fixes error on `|` character in CLI

Open rishabhshuklax opened this issue 5 years ago • 12 comments

Fixes #1033 Fixes #1040

This PR fixes error on | character in CLI inputs

The supported syntax after this PR would be

./index.js -s "contrast{contrast:55},blur{blur:2.4}" -i examples/images/test.png

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • [x] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm run test-all
  • [x] code is in uniquely-named feature branch and has no merge conflicts
  • [x] PR is descriptively titled
  • [x] ask @publiclab/is-reviewers for help, in a comment below
  • [x] Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to 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!

rishabhshuklax avatar Feb 21 '20 10:02 rishabhshuklax

@publiclab/is-reviewers @jywarren please review this!

rishabhshuklax avatar Feb 21 '20 10:02 rishabhshuklax

Codecov Report

Merging #1634 into main will decrease coverage by 1.79%. The diff coverage is 47.16%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/CliUtils.js 37.50% <0.00%> (ø)
src/Modules.js 100.00% <ø> (ø)
src/modules/EdgeDetect/Module.js 100.00% <ø> (ø)
src/modules/WebglDistort/Module.js 2.29% <0.00%> (ø)
src/modules/ColorHalftone/Module.js 3.63% <3.63%> (ø)
src/modules/Overlay/Module.js 93.18% <94.11%> (-1.69%) :arrow_down:
src/modules/ColorHalftone/index.js 100.00% <100.00%> (ø)
src/modules/Colorbar/Module.js 100.00% <100.00%> (ø)
src/modules/Crop/Module.js 88.88% <100.00%> (ø)
src/modules/EdgeDetect/EdgeUtils.js 86.81% <100.00%> (-0.15%) :arrow_down:
... and 7 more

codecov[bot] avatar Feb 21 '20 10:02 codecov[bot]

Can you explain, in brief, what caused this and how you fixed it? Thanks.

harshkhandeparkar avatar Feb 22 '20 16:02 harshkhandeparkar

@HarshKhandeparkar The error was because the syntax for taking input in CLI was

./index.js -s "webgl-distort" -c '{"nw":"100,100","se":"40,40"}' -i examples/images/test.png

So I rewrote how the inputs were taken to support the following syntax

./index.js -s "webgl-distort{nw:100%2C100|se:40%2C40}" -i examples/images/test.png

Similar to how we take inputs from URL hash.

rishabhshuklax avatar Feb 23 '20 09:02 rishabhshuklax

Oh so the CLI and URL input formats werr different.

harshkhandeparkar avatar Feb 25 '20 14:02 harshkhandeparkar

gitpod-io[bot] avatar Jul 07 '20 14:07 gitpod-io[bot]

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!

image

jywarren avatar Jul 07 '20 14:07 jywarren

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.

jywarren avatar Jul 10 '20 21:07 jywarren

Noting that we should also link this to https://github.com/publiclab/image-sequencer/issues/1694 !

jywarren avatar Jul 10 '20 22:07 jywarren

Shall we merge this after https://github.com/publiclab/image-sequencer/pull/1718, the CLI tests? Thanks!

jywarren avatar Oct 16 '20 21:10 jywarren

Noting CLI tests now exist:

image

jywarren avatar Nov 01 '20 16:11 jywarren

Hi all, just wondering what the status of this is now -- thank you!!!

jywarren avatar Feb 19 '21 19:02 jywarren