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 for #218 #219

Merged
merged 4 commits into from Nov 29, 2017

Conversation

Projects
None yet
2 participants
@Aerijo
Contributor

Aerijo commented Nov 28, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

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.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 28, 2017

Member

@Aerijo can you update the failing specs please?

Member

50Wliu commented Nov 28, 2017

@Aerijo can you update the failing specs please?

@Aerijo

This comment has been minimized.

Show comment
Hide comment
@Aerijo

Aerijo Nov 28, 2017

Contributor

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

Contributor

Aerijo commented Nov 28, 2017

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

@Aerijo

This comment has been minimized.

Show comment
Hide comment
@Aerijo

Aerijo Nov 28, 2017

Contributor

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.

Contributor

Aerijo commented Nov 28, 2017

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.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 28, 2017

Member

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.

Member

50Wliu commented Nov 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Aerijo

Aerijo Nov 28, 2017

Contributor

@50Wliu All checks passed

Contributor

Aerijo commented Nov 28, 2017

@50Wliu All checks passed

@50Wliu 50Wliu merged commit 52bb626 into atom:master Nov 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu referenced this pull request Nov 29, 2017

Closed

Parentheses included in link scope #218

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment