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

buildBinding regex breaks with multiple strings #620

Closed
agmonks opened this issue May 25, 2016 · 10 comments
Closed

buildBinding regex breaks with multiple strings #620

agmonks opened this issue May 25, 2016 · 10 comments

Comments

@agmonks
Copy link
Contributor

agmonks commented May 25, 2016

This is broken in 0.9 (fixed as part of #432)

/((?:'[^']*')*(?:(?:[^\|']+(?:'[^']*')*[^\|']*)+|[^\|]+))|^$/g

This breaks for the following example (not picking up the 2nd string):

receiptTypeId | lookup reciptTypes 'value' 'key'

I'm trying to come up with a working regex but I'm not great at regex!

@jccazeaux
Copy link
Contributor

Damn, didn't see that 😞
I found a better regexp. It works fine but returns empty strings. Didn't find a way to eliminate these in the regexp. So i eliminated them with a filter. PR will follow, if anyone has a better idea i'll happy to see it !

@agmonks
Copy link
Contributor Author

agmonks commented May 25, 2016

I came up with the same regex issues with the empty spaces. Would recommend not using filter as for loop would work quicker (don't think the extra checks filter runs are needed here)

@jccazeaux
Copy link
Contributor

I agree, the code is work in progress. I hoped someone could help to find THE regexp ;-)
In the meantime feel free to propose a better/faster solution.

@agmonks
Copy link
Contributor Author

agmonks commented May 26, 2016

How about:

((?:'[^']*')*(?:(?:[^\|']*(?:'[^']*')+[^\|']*)+|[^\|]+))|^$

@jccazeaux
Copy link
Contributor

Looks great!
I'll close m'y PR, feel free to send a PR with your solution (won't be able to do it until 2 weeks)

agmonks pushed a commit to agmonks/rivets that referenced this issue May 31, 2016
@agmonks
Copy link
Contributor Author

agmonks commented May 31, 2016

@Leeds-eBooks This issues probably justifies a 0.9.1 release (as it might break other peoples apps like mine)?

@benadamstyles
Copy link
Collaborator

0.9.1 published!

@agmonks
Copy link
Contributor Author

agmonks commented May 31, 2016

Excellent! Thanks

agmonks pushed a commit to agmonks/rivets that referenced this issue May 31, 2016
agmonks pushed a commit to agmonks/rivets that referenced this issue May 31, 2016
@agmonks
Copy link
Contributor Author

agmonks commented May 31, 2016

@Leeds-eBooks FYI I have also created a ES6 fix (PR #626) as well (also had to fix up the testing framework in PR #625)

@jccazeaux
Copy link
Contributor

Great ! Thanks !

blikblum added a commit to blikblum/tinybind that referenced this issue Aug 1, 2016
benadamstyles added a commit that referenced this issue Aug 7, 2016
Fixed binding to allow multiple strings, ES6 version of #624 Fixes #620
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

No branches or pull requests

3 participants