-
Notifications
You must be signed in to change notification settings - Fork 12
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
The Italic Conundrum #24
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A quirk of the engine allows the old to somewhat work but it should be looking before the `_`.
This prevents the case of the rule from the main grammar having already being entered and not being able to exit as expected.
This was improperly matching the first half of the possible closing `__`.
As this would be raw text nothing should be matched here.
Thanks for the PR @infininight. I'll let @noniq chip in here as he's dug into this far more than I have. |
Looks perfect, thanks so much @infininight! I wonder if it would make sense to add a small test document containing an example of each of the cases your PR is addressing? Just to make it easier to avoid regressions in the future! |
Added a few I was using during testing. |
Merged, thanks again! |
Thanks both! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous fix for this issue uses the regex:
This puts the look-behind assertion after the
_
looking back, this shouldn't ever match but does due to some quirk of the engine which I don't really understand. I've moved this before the_
character to make it function as intended.Edit: Brain fart moment there, of corse it worked because
_
is a word character, silly me. Regardless, the intent would be to match\w
before the_
so it should be moved.There are other issues though: The first is due to how the italics rule in the main grammar is done. It uses a look-ahead to ensure that the ending will exist before starting, it can easily be tripped up with the string:
Because the italics rule is entered due it detecting a valid italics context but the injected grammar yanking the closing
_
leaving the context persisting indefinitely.Started looking into this due to some discussion in textmate/markdown.tmbundle#46 with the MathJax string:
The proper way to handle this specific case I believe is a MathJax grammar, but it was a real world example of the issue.
To improve the issue I've again disabled the rule inside italics contexts, which allows a rule already entered to exit. The rule now only serves to prevent a italics context to be entered in a common invalid context for GitHub's markdown variant.
I think to truly solve this issue a rule would need to be written to wholly match 'word' like constructs that are not desired to be passed through the main italics rule.
An example of where the behavior in this PR fails is the first example again:
Where GitHub does not consider this to be italics as the closing
_
is inside the rule but we can't exclude the case as we are already too late with a simplistic exception that hasn't matched a larger section of the text.A second issue easily fixed is that it was matching the third
_
in the string:Which prevents the bold context to exit, this has been corrected.
Also corrected is properly excluding embedded and raw cases as it should never match characters in those.
cc @noniq