Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix HTML Entities regex #142

Merged
merged 3 commits into from
Nov 3, 2016
Merged

Fix HTML Entities regex #142

merged 3 commits into from
Nov 3, 2016

Conversation

JonathanWolfe
Copy link
Contributor

Caused JonathanWolfe/language-polymer#1. Just need a simple lookahead assertion.

was too greedy and broke other grammars
@hediyi
Copy link
Contributor

hediyi commented Oct 16, 2016

Could you also update the specs?

@@ -363,7 +363,7 @@
'entities':
'patterns': [
{
'begin': '(&)([a-zA-Z0-9]+|#[0-9]+|#x[0-9a-fA-F]+)'
'begin': '(&)([a-zA-Z0-9]+|#[0-9]+|#x[0-9a-fA-F]+)(?=;)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of rewriting the grammar to

        'match': '(&)([a-zA-Z0-9]+|#[0-9]+|#x[0-9a-fA-F]+)(;)'
        'captures':
          '1':
            'name': 'punctuation.definition.entity.begin.html'
          '2':
            'name': 'entity.name.entity.other.html'
          '3':
            'name': 'punctuation.definition.entity.end.html'
        'name': 'constant.character.entity.html'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the better way forward since entities aren't able to wrap around anything.

@JonathanWolfe
Copy link
Contributor Author

Swapped to use match instead of the begin/end semantic. Also corrected spec changes.

@winstliu
Copy link
Contributor

/cc @DavidePastore I may have to revert your change in order to fix the recent regressions regarding ampersands (such as #141). Any suggestions?

@DavidePastore
Copy link
Contributor

@50Wliu thanks for the cc. The fact is that &a has a particular meaning in my implementation: you're in the middle of an entity and you could use this information to trigger autocomplete. Maybe we could try to preserve the first pattern and edit the second.

@JonathanWolfe
Copy link
Contributor Author

@DavidePastore - I'm not a regex guru but I can't think of any way to have that functionality and not break the rest of the spec. It will constantly try to eat the rest of the document (or line at a minimum with a change) thinking you're still writing an entity. It is possible to preserve autocomplete but it would require you to write &; then go back and fill in the entity.

@winstliu
Copy link
Contributor

winstliu commented Nov 3, 2016

I'm reverting this for now and will investigate alternative options later.

@winstliu winstliu merged commit 06a3202 into atom:master Nov 3, 2016
@JonathanWolfe JonathanWolfe deleted the patch-1 branch November 3, 2016 15:05
@DavidePastore
Copy link
Contributor

@JonathanWolfe - Yes, you're right. I'm not a regex guru either.
@50Wliu You did the right thing 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants