search

atom / language-gfm

GitHub Flavored Markdown in Atom
MIT License
101 stars 108 forks source link

Fix for #218 #219

Closed Aerijo closed 7 years ago

Aerijo commented 7 years ago

Requirements

Description of the Change

As per #218, this PR fixes the scoping of links in patterns such as ![text](link)

The changes are relatively minor. Each relevant regex used to have a capture group around the parentheses and the contents, and this group was scoped to markup.underline.link. My PR removes this capture group (as it was not used otherwise) and introduces a new one strictly around the contents of the parentheses.

Alternate Designs

I could have left the outer capture group in there and introduced an entirely new one, but that would have meant reworking a lot of group numbers. As my solution effectively "moved" the group, only a couple of group numbers were affected.

Benefits

Links are properly scoped to just the part that is actually a link. Note that I don't understand some how some of the link syntax works, I've just been going off what was already labelled as a link.

Fixed constructions: 

[![text1](link1)](link2)
[![text1](link1)][link2]

[![text1][link1]][link2]
[![text1][link1]](link2)

![text1](link1)
[text1](link1)

![text1][link1]
[text1][link1]

[text1]:<link --- or not link?
[text1]:<link <!-- it breaks comments :( -->
[text1]:<link1>

[text1]:link1

[text1](link1)

Possible Drawbacks

None I am aware of. As stated, the removed capture groups were not used anyway. Someone needing them in the future would need to put some effort in to correct group numbers, but I don't expect this to happen.

Applicable Issues

218

Other notes

Seen in the benefits section, there are some patterns (already present; not introduced or addressed by this PR) that highlight counterintuitivly. Eg. does [text]:<link need a closing >?

I also dont know much about testing, so any help with that (if necessary for this change) would be appreciated.

winstliu commented 7 years ago

@Aerijo can you update the failing specs please?

Aerijo commented 7 years ago

I don't know why it's failing or how to do that :(

Aerijo commented 7 years ago

I looked at the specs. Just a sanity check: is there any reason to include parentheses as part of the link? Because it looks like someone explicity told the specs to expect ( and ) to be scoped as a link.

winstliu commented 7 years ago

No. We often write the specs to exactly match the grammar so that unintentional changes are caught. So they can definitely be changed in this case.

Aerijo commented 7 years ago

@50Wliu All checks passed