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

add test case for nested typed expression #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ksheedlo
Copy link

@ksheedlo ksheedlo commented May 2, 2015

This change adds a test case for #62. Stepping through in the debugger
suggests that both selectors are run against the same AST. Since the
_str$s variable matches an AST nested inside the $e expression and
the $e expression appears first, the intended _str$s is not found.

This change adds a test case for gkz#62. Stepping through in the debugger
suggests that both selectors are run against the same AST. Since the
`_str$s` variable matches an AST nested inside the `$e` expression and
the `$e` expression appears first, the intended `_str$s` is not found.
@ksheedlo
Copy link
Author

ksheedlo commented May 2, 2015

Not to be merged until a fix is found, since this will break the build.

@ksheedlo
Copy link
Author

ksheedlo commented May 2, 2015

I'd be interested to see if this problem can be solved while keeping the existing replacement algorithm using String.prototype.replace. One way I could see trying to do it would be to replace AST nodes that have already been matched against with some special marker, so that the replacement algorithm can ignore them as they have already been matched against.

Another way to do it would be to compile the search and replacement strings and have a function to match against the AST all at once, but this would likely require an overhaul of the existing algorithm.

@gkz
Copy link
Owner

gkz commented May 3, 2015

Thanks for looking into this. I'm focused on getting the next version of LiveScript out, but after that I'll take a look at the various Grasp issues.

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