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

test import string with no configs #279

Merged
merged 8 commits into from
Jun 3, 2018

Conversation

jywarren
Copy link
Member

@jywarren jywarren commented Jun 3, 2018

No description provided.

@ghost ghost assigned jywarren Jun 3, 2018
@ghost ghost added the in-progress label Jun 3, 2018
@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren we have to have the(), they are essential

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

I don't think they are -- if we are URL-encoding the commas within functions, we can then do split(',') to separate steps.

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren yes we can, if you want I'll do it this way, just some changes please give me five minutes!!

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

Actually @jywarren what happens is we use ( to separate step name and it parameters

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

The problem is that we need commas to separate the parameters, so we can't rely on , to separate just steps. This makes me think we ought to just use pure JSON... but can you think of another solution?

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

We could separate parameters with pipes |?

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren No problem, I have a work around!!

@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

No need of pure json

Signed-off-by: tech4GT <[email protected]>
@tech4GT
Copy link
Member

tech4GT commented Jun 3, 2018

@jywarren Please have a look at the new pr that fixes our problem!!

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

OK, i pulled it but I think there's one more thing odd with it -- see how the test returns:


    ✖ should be equivalent
    -----------------------
      operator: deepEqual
      expected: |-
        [ { name: 'channel', options: { channel: 'green' } }, { name: 'blur', options: { blur: '2' } }, { name: 'invert', options: {} }, { name: 'blend', options: { blend
: 'function(r1, g1, b1, a1, r2, g2, b2, a2) { return [ r1, g2, b2, a2 ] }' } } ]
      actual: |-
        [ { name: 'green', options: {} }, { name: 'blur', options: {} }, { name: 'invert', options: {} }, { name: 'blend', options: {} } ]
      at: Test.<anonymous> (/home/warren/sites/image-sequencer/test/modules/import-export.js:27:5)
      stack: |-

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

But great work!

@jywarren jywarren mentioned this pull request Jun 3, 2018
@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

I refactored a bit for readability -- but I'm still seeing an error -- i believe it's still because it can't distinguish between commas used to separate steps, and commas used to separate step settings... I think we ought to switch to pipes, because it's tough otherwise!

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

I mean we could switch to semicolons...

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

OK, this should work!!!

@jywarren
Copy link
Member Author

jywarren commented Jun 3, 2018

Hooray! OK awesome. Read through these changes, but they're pretty minimal!

@jywarren jywarren merged commit 37122d4 into publiclab:master Jun 3, 2018
@ghost ghost removed the in-progress label Jun 3, 2018
@jywarren jywarren mentioned this pull request Jan 4, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants