Skip to content

Conversation

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Sep 25, 2019

The old ruleset only matched "by accident". I imagine the 2nd rule would trigger from the termination match and then when the tiny "```" lexeme was processed the first rule would now incorrectly trigger since it just happens to match even despite the typos.

Ditto for the end tag... otherwise I can't see how it was ever working.

@marcoscaceres
Copy link
Contributor

Thanks for doing this fix @yyyc514. Any chance you could add a test?

@joshgoebel
Copy link
Member Author

There is no test because it matched by accident before. The behavior was correct DESPITE the rule being wrong.

@joshgoebel
Copy link
Member Author

Since this more in the realm of "typo" I don't imagine it's a regression that will reoccur in the future (at least not this individual issue). Improving the parser itself might find other cases of this, but that's outside the scope of this issue. :-)

@egor-rogov
Copy link
Collaborator

Funny.
'\s*\w*' piece becomes s*w* and is simply ignored due to its optionality. And $ is also ignored (but why?) in begin. So this works just fine (:
And \t becomes tabulation char, which is OK.

I agree that this is kinda typo, and is in fact covered by existing test. Sure it's better to be fixed.

One question, @yyyc514: why have you changed \s to [ ] and not to \\s?

@joshgoebel
Copy link
Member Author

joshgoebel commented Sep 25, 2019

And $ is also ignored (but why?) in begin

It's ignored because inside processLexeme it IS The end of the string (because the second rule was the one matching I'd imagine)... and the second rule doesn't have a $, not it works for the first match, then once that gets inside processLexeme the broken rule will now match..

It would probably break if you had 3 backticks followed by a literal "s" though. :-)

This is the flaw with running the rules twice. All sorts of strange edge cases. :-)

One question, @yyyc514: why have you changed \s to [ ] and not to \s?

\s will match newlines and the test result said the </span> was supposed to IMMEDIATELY follow the end of the backticks (and this seems more correct to me), not swallow the newline first, which would have put it on the following line.

@egor-rogov
Copy link
Collaborator

OMG. I should dig deeper into this...
Okay, let fix it this way for now, although it seems this language is ought to be reworked someday.

@egor-rogov egor-rogov merged commit 7287eec into highlightjs:master Sep 26, 2019
@joshgoebel joshgoebel deleted the syntax_fixes branch February 15, 2020 12:59
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.

3 participants