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

Prevent <<command("parameters",123)>> from crashing bondage.js #40

Merged
merged 7 commits into from
Sep 28, 2018
Merged

Prevent <<command("parameters",123)>> from crashing bondage.js #40

merged 7 commits into from
Sep 28, 2018

Conversation

blurymind
Copy link
Collaborator

@blurymind blurymind commented Sep 19, 2018

Gave this another shot - this time from a different angle. Much thanks to @jhayley for the kind feedback and patience :)

There is still a quirk seen here:
https://github.com/jhayley/bondage.js/pull/40/files#diff-1f6fe15287b398db63504fd4a8ee9bbbR404
Not sure why bondage.js adds an extra space before the "("

@hylyh
Copy link
Owner

hylyh commented Sep 20, 2018

This is looking good so far! The space thing is an issue though, I think it's related to this line https://github.com/jhayley/bondage.js/blob/40748d2e50e26f40398df0a01e4d0bc070fe35e6/src/parser/parser.js#L116

@blurymind
Copy link
Collaborator Author

Ah yes, I think you're right. I had my suspicions, but now its pretty clear. I will try to tackle this tonight :)

@blurymind
Copy link
Collaborator Author

@jhayley fixed now and passing all tests :) even the new tests

src/lexer/tokens.js Outdated Show resolved Hide resolved
also extend the tests a little more
This greatly simplifies command parsing and allows spaces inside <<>> without any ugly hacks
@blurymind
Copy link
Collaborator Author

blurymind commented Sep 22, 2018

@jhayley After ranting with myself for a bit here, I eventually created a new 'CommandCall' token and removed the ugly hack altogether. All tests passing, simplified command parser code and anything allowed inside << >>, even spaces - without using that workaround that artificially inserts spaces

My regular expression basically takes anything that is not >> and allows it

Best part is, we no longer need this line:

      /// Extremely ugly hack because a command with spaces (e.g. <<foo bar>>)	
      /// Lexes as BeginCommand Identifier Text EndCommand	
      ['BeginCommand Identifier Text EndCommand', '$$ = new yy.CommandNode($2 + " " + $3);'],

@hylyh hylyh merged commit d851e9e into hylyh:master Sep 28, 2018
@hylyh
Copy link
Owner

hylyh commented Sep 28, 2018

Looks good! Will push to npm when I have a chance

@blurymind
Copy link
Collaborator Author

blurymind commented Sep 28, 2018 via email

@hylyh
Copy link
Owner

hylyh commented Sep 29, 2018

pushed! ✨

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