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

Fix corner cases for the greedy flag #1095

Merged
merged 2 commits into from
May 8, 2017

Conversation

zeitgeist87
Copy link
Collaborator

By refactoring the tokenize() method into two separate methods, it is
possible to recursively clean up any broken tokens left by the greedy
feature.

This should fix the issue #1075.

@zeitgeist87
Copy link
Collaborator Author

Actually this is not a good solution after all. It only works if the comment contains another comment.

@zeitgeist87
Copy link
Collaborator Author

The same error occurs with a template string in JS:

greedy

@Golmote
Copy link
Contributor

Golmote commented Feb 8, 2017

Thank you for taking a look into this!
I think your idea should be able to fix even the template string example. After the part //" ` sdsf ` is invalidated, we could reprocess only the remaining fragment, that is ` sdsf `, which should highlight as a template string, right?

@zeitgeist87
Copy link
Collaborator Author

Yes I think it is possible to optimize and generalize this approach. I will try a few things and then update this PR.

@zeitgeist87
Copy link
Collaborator Author

I don't think we can make the greedy flag perfect. It was a compromise from the beginning and there will always be a few corner cases where it fails.

At some point we have to decide if the extra effort of fixing greedy is worth it.

@zeitgeist87
Copy link
Collaborator Author

It should be fixed now. I've also added test cases to a few languages including one test case for template strings in Javascript.

@zeitgeist87
Copy link
Collaborator Author

@Golmote Do you think this can be merged?

@Golmote
Copy link
Contributor

Golmote commented May 8, 2017

@zeitgeist87 Again sorry for the long time before review. This looks good to me. Thank you so much for digging into this.

Feel free to merge when you can!

By refactoring the tokenize() method into two separate methods, it is
possible to recursively clean up any broken tokens left by the greedy
feature.

This should fix the issue 1075.
@zeitgeist87
Copy link
Collaborator Author

@Golmote Thanks for the review! I've rebased it and solved the merge conflicts.

@zeitgeist87 zeitgeist87 merged commit 6530709 into PrismJS:gh-pages May 8, 2017
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.

None yet

2 participants