Close parenthesis fix

[vibhorgupta-gh]

Fixes #459

• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
• code is in uniquely-named feature branch and has no merge conflicts
• PR is descriptively titled
• ask @publiclab/reviewers for help, in a comment below

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

[vibhorgupta-gh]

@jywarren there was a slight tweak in the regex, the end braces now appear in the options. Checked against this:

http://localhost:3000/examples/#steps=dynamic{red:map(r%2C0%2C255%2C0%2C128)|green:g|blue:b|monochrome%20(fallback):r%20%2B%20g%20%2B%20b}

@tech4GT please review :)

[harshkhandeparkar]

@VibhorCodecianGupta the paranthesis "{ }" don't need to be escaped "\{" in regex.

[vibhorgupta-gh]

@harshkhandeparkar that was already in place, I made changes to the closing braces only. Should I remove the ^ from ^\{ ?

[harshkhandeparkar]

^ means the start of line. If we remove that, the pattern will still work as it only matches the first appearance but it is used for security. \ before the { is not needed as { matches itself literally in regex. But if its is used like {2} then it is a quantifier.

[harshkhandeparkar]

I actually didn't understange your change. How can } be at the start and also end of string in /^\}$/ ? Doesn't it mean that it only matches a single character string?

[harshkhandeparkar]

By the way, what settingValue can have a { at the start?

[vibhorgupta-gh]

@harshkhandeparkar I tested out some possibilities, and it turns out that using the replace statement is not really required. The substring extracted already has () which are also necessary for the function in Dynamic module to run. The .replace functions can be done away with, considering we need the brackets for the function to run, like so map(r,0,255,0,128). Extracting the relevant substring and then decoding it should do the job.
Am I missing something here?

[harshkhandeparkar]

Ya exactly, I felt the same way which is why I asked you the question. The replace might be redundant as it never finds a match but there may be other functions which have double parenthesis. We have to confirm this with @tech4GT or @jywarren.

[harshkhandeparkar]

Maybe replacing { } is important but not ( ).

[vibhorgupta-gh]

@harshkhandeparkar cool then. For now I'll update the pr with working commits, and proceed as mentors guide

[vibhorgupta-gh]

@jywarren @tech4GT please advise

[jywarren]

Hmm, ok -- maybe we need to take a step back and:

1. write a test that demonstrates the original function of these replace methods (in positive and negative forms)
2. write a test which demonstrates the bug I found in Close-parentheses in Dynamic module input get lost when sourced from URL #459

That'll ensure we know why it was originally created and we won't break the original purpose while fixing the new bug.

I looked at the blame:

https://github.com/publiclab/image-sequencer/blame/master/src/ImageSequencer.js#L308-L309

It led me back to #313

that made it possible to use {} as well as () to add parameters to a string formatted sequence. but original () code is here:

#279

I believe the original is attempting to make it possible to use {} to delimit parameters in string format, especially when passing a function in a parameter, as we are doing with map(). So maybe the original test needed is to pass in a string with a function as a parameter like map(), and confirm that the instantiated step has a complete but not malformed function -- that it preserved the () properly when adding the steps from the original string.

Then we can debug what happens here. How does that sound?

[codecov]

Codecov Report

Merging #580 into main will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##             main     #580   +/-   ##
=======================================
  Coverage   37.04%   37.04%           
=======================================
  Files         100      100           
  Lines        1857     1857           
  Branches      291      291           
=======================================
  Hits          688      688           
  Misses       1169     1169

Impacted Files	Coverage Δ
src/Strings.js	18.07% <0%> (ø)	⬆️

[vibhorgupta-gh]

@jywarren after looking at the aforementioned issues, and logging some outputs, this is what is happening:

As can be seen, in favour of supporting #306 , the parenthesis was also expected to be removed from the string passing the steps and options. That works fine, except for the map() function which requires both the braces and parenthesis and hence the ending ) gets omitted along with }, which is undesirable. For now, the latest commit removes the replace function that removes the ending ) to support the map function, like so:

This solution, however, completely depends on whether we still want IS to support the old syntax of using () instead of {}. In that case, I'll have to come up with some other logic.

[jywarren]

Ah... I see. I guess this is a good reason to drop () then... what do you think? I could imagine trying to develop a more sophisticated parser, perhaps using regex, but I'm not sure how possible it is. And perhaps a simpler one with a simpler string syntax is the better solution?

…

On Wed, Apr 17, 2019, 12:28 PM Vibhor Gupta ***@***.***> wrote: @VibhorCodecianGupta <https://github.com/VibhorCodecianGupta> requested your review on: #580 <#580> Close parenthesis fix. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#580 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ_aXEZA-eCIIeLpN7vANmJO6gCxBks5vh0t6gaJpZM4ZlRQ6> .

[vibhorgupta-gh]

I guess we could drop the () then, and update the docs accordingly, explicitly mentioning that the said syntax is deprecated. Is it being used extensively in some other library leveraging IS? If not, this shouldn't be too much of a problem.
A more sophisticated parser as in? Didn't quite get you. I think the current one uses regex as well and does pretty well

[jywarren]

Let's drop the (); I don't think any other apps are using it because it was from very early on. Thanks!!!

[jywarren]

Nevermind about the parser... I think there's a way to "find matching parentheses" working from the outside in, but I really think it's not worth it even if it is possible. Thanks a lot!

[vibhorgupta-gh]

@jywarren in that case, I think this is good to go. I'll just fix the conflicts then, and update the docs?

[jywarren]

Great!!!

[vibhorgupta-gh]

@jywarren can we make this a part of the next bump as well?

[jywarren]

Merging this now! We'll try to pick it up in v3.3.2. Thanks!