atuttle / atom-language-cfml

:space_invader: A CFML Language for the Atom Editor
38 stars 24 forks source link

Add another failing hyperlink highlighting test case #60

Closed elpete closed 9 years ago

elpete commented 9 years ago

Good news: language-hyperlink pushed a new version. It fixed one of our test cases!

Bad news: There are some other test cases I missed. 🙁 Here they are.

atuttle commented 9 years ago

:+1: tests ftw

tollus commented 9 years ago

Should we just make the regex not match any links with 2+ # characters in it?

elpete commented 9 years ago

Are you talking about the regex for language-hyperlink? That would be lovely. I've been trying to do that, but regex is definitely not my strong point.

The other option is to exclude language-hyperlink on matching any cfml strings, but that seems overboard.

elpete commented 9 years ago

Additionally, @tollus, this really should be not matching any links with 2+ # characters within the same / characters. For instance, the link: http://localhost:8000/#/tasks#completed is a valid hyperlink outside the cfml world (for single page apps and the like).

tollus commented 9 years ago

Yeah, that's what I was thinking. And that's a good point, I guess you can't be that explicit.

tollus commented 9 years ago

Though if this is correct, you're not "supposed to" have a # in a URL hash?

http://stackoverflow.com/a/2849800/1454642

elpete commented 9 years ago

Got me. I've seen the # used in url histories where browser support is old.

One last point is that we could introduce a separate grammar file on lanugage-hyperlink specifically for cfml files.

In any case, the first step is to get a good regex for don't match if there are two # signs anywhere.

tollus commented 9 years ago

Going back to the original regex, here's a regex to match only URLs that do not have multiple # in them:

'patterns': [
  {
    'match': '(?x)(?:(?!(?:.+\\#){2,})( (https?|s?ftp|ftps|file|smb|afp|nfs|(x-)?man(-page)?|gopher|txmt|issue):\/\/|mailto:)[-:@[:word:].,~%+\/?=&#;|!]+(?<![-.,?:#;]))'
    'name': 'markup.underline.link.$2.hyperlink'
  }

won't match:

will match

My only question is, should the regex match the http://github.com/ part of the first won't match.

Should we take this discussion over to the hyperlink repo?

elpete commented 9 years ago

Yes! Do you want to submit the pull request over there? Make sure to adjust the tests to show that cfml strings will not be matched. Good work. We'll discuss whether we should match some or none on that pull request.