Skip to content
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

Error in CLI parsing of options #1033

Open
jywarren opened this issue Apr 19, 2019 · 8 comments · May be fixed by #1634
Open

Error in CLI parsing of options #1033

jywarren opened this issue Apr 19, 2019 · 8 comments · May be fixed by #1634

Comments

@jywarren
Copy link
Member

It seems like the | which separates options in the string syntax are not getting read properly, causing sequencer to think they are part of a new type of module name, perhaps? See this error below:

(xenial)warren@localhost:~/sites/image-sequencer$ ./index.js -s webgl-distort{nw:100,100|se:40,40} -i examples/images/test.png
bash: se:40,40}: command not found
Can't read file.

@VibhorCodecianGupta i know you are quite busy already but could this potentially relate to the new string parsing work? Potentially we could ask someone else to help out here as well, I don't want to ask too much of you at once! Thanks a lot!

Both this and #1032 have come up while trying to run the new webgl-distort module in Google Cloud, so solving them both will open up some pretty exciting new use cases. cc @tech4GT

Thanks!!!

@jywarren
Copy link
Member Author

Hmm, another datapoint... I opened this url, and the URL stays the same:

https://sequencer.publiclab.org/examples/#steps=crop{x:260|y:110|w:50|h:50},average,import-image,crop{x:500|y:110|w:50|h:50},average

But the values in the options UI don't show the url values. I'm on a phone so I can't confirm but I wonder if it's showing the same parse error for | characters?

We could write a test for this in Jasmine now, starting with a url and confirming that the parameters are passed through!

@jywarren
Copy link
Member Author

@publiclab/is-reviewers I'd love some help with this one if anyone is interested!!!

@harshkhandeparkar
Copy link
Member

I'll look into it tonight.

@vibhorgupta-gh
Copy link

vibhorgupta-gh commented Apr 20, 2019

@jywarren

  1. The syntax you mentioned,
./index.js -s webgl-distort{nw:100,100|se:40,40} -i examples/images/test.png

This is not yet supported for the CLI syntax. We only added the support for importing input strings in a node environment (and not CLI). If this is desirable for other projects, shall I add it here? This and the support for JSON modules?

The currently supported syntax is: (which is pure JSON, not having |)

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

but this also works, although it throws a JSON syntax error. Produces the correct image, though:

./index.js -s "webgl-distort" -c '{"nw":100,100|"se":40,40}' -i examples/images/test.png
  1. The chained URL
http://localhost:3000/examples/#steps=crop{x:260|y:110|w:50|h:50},average,import-image,crop{x:500|y:110|w:50|h:50},average

works fine for me in local environment and also works fine in publiclab.org :D. Might've been some issue when you tried out?

Screenshots:

Screen Shot 2019-04-20 at 11 50 58 AM

Screen Shot 2019-04-20 at 11 51 06 AM

@jywarren
Copy link
Member Author

Hmm, you're right that the colorimetry use case is working on gh-pages. Thanks!

Also, I think i was wrong that the CLI is the cause of issues in gcloud, thanks for clarifying. I jumped to conclusions about this error:

publiclab/image-sequencer-app#6 (comment)

So, while we can leave the CLI issue open because we /should/ solve it, the more urgent thing is the way it's failing to parse in the GCloud environment. Opening something more specific about that now!

@jywarren
Copy link
Member Author

OK, moved here with more specifics! #1040

@tech4GT
Copy link
Member

tech4GT commented May 17, 2019

Okay is this still failing @VibhorCodecianGupta
Do you need help here??

@jywarren jywarren added this to the Core improvements milestone May 25, 2019
@rishabhshuklax
Copy link
Member

@jywarren is this issue still open?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants