Skip to content

More conservative link matching.#57

Merged
Alhadis merged 2 commits intoLukasa:masterfrom
phyllisstein:master
Nov 22, 2017
Merged

More conservative link matching.#57
Alhadis merged 2 commits intoLukasa:masterfrom
phyllisstein:master

Conversation

@phyllisstein
Copy link
Copy Markdown
Contributor

@phyllisstein phyllisstein commented Nov 21, 2017

Kind of an edge case, but including TypeScript snippets with tagged template literals previously broke the highlighting, causing everything in the file after the closing ` to be treated as a link title. This slightly more conservative match seems to cover the most common way links are used without matching them promiscuously across lines.

Before

before

After

after

@phyllisstein
Copy link
Copy Markdown
Contributor Author

Also added some changes to add TypeScript to the embedded syntaxes and fix the end-of-block matching. To the best of my knowledge, using \1 in the end regex would only create a backref to that expression; there's no way to reach through to recover the begin pattern's matches. The change in 5795ddd is, comparatively, way less clever and more naïve, but it works:

Before

before

After

after

@Alhadis
Copy link
Copy Markdown
Collaborator

Alhadis commented Nov 21, 2017

Looks good. Just one small thing:

To the best of my knowledge, using \1 in the end regex would only create a backref to that expression; there's no way to reach through to recover the begin pattern's matches

There is, and that's how the TextMate highlighting system implements features like heredocs and Markdown code-fences. Sharing of capturing groups is somewhat of a "magic feature" that's limited to begin/end pairs only.

The capture it's back-referencing is the leading indentation of the beginning match; which is how we're able to nest one indented component inside a larger indented component.

But everything else looks good!

@phyllisstein
Copy link
Copy Markdown
Contributor Author

Gotcha! TMYK🌠. Shall I back that commit out, then, and play with the regexes a bit more? It definitely wasn't matching as expected, but that's arguably a problem for another PR.

@Alhadis
Copy link
Copy Markdown
Collaborator

Alhadis commented Nov 21, 2017

Probably better to leave it for a different PR, I'd say.

@phyllisstein
Copy link
Copy Markdown
Contributor Author

👍 Duly dropped! I'll see if I can sort through the regex issue this weekend. Thanks for the feedback!

@Alhadis Alhadis merged commit 32b9912 into Lukasa:master Nov 22, 2017
@Alhadis
Copy link
Copy Markdown
Collaborator

Alhadis commented Nov 22, 2017

I'll hold off on cutting a release until you get back to me. =)

@Alhadis
Copy link
Copy Markdown
Collaborator

Alhadis commented Dec 4, 2017

Hey, @phyllisstein, how did you go with tackling the other half of this PR?

I'd like to get another release cut sometime soon-ish, and it'd be great if we could get this fix on-board too.

@phyllisstein
Copy link
Copy Markdown
Contributor Author

phyllisstein commented Jan 20, 2018

I'm so sorry for not getting back to you sooner. I'm actually not able to reproduce the issue with end captures in a recent build of Atom---could it be that they fixed something on their end while I was lollygagging?

screenshot 1

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