Skip to content
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

Use Atom's HTML grammar instead of TextMate's #4274

Merged
merged 2 commits into from
Sep 25, 2018
Merged

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Sep 25, 2018

This PR replaces the grammar used for HTML highlighting on GitHub.

Description

This is a follow-up to mariozaizar/language-eml#15, where @pchaigno observed jarring error highlights being applied to HTML embedded in another grammar (see #4201). The spurious highlights in question are absent when using Atom's HTML grammar for highlighting.

Moreover, Atom's grammar has greatly improved visibility than TextMate's, and is likely to stay updated faster as the HTML spec itself evolves.

Checklist:

  • I am changing the source of a syntax highlighting grammar

No real difference in highlighting, apart from when embedded content is involved. Which, unfortunately, is impossible to demonstrate due to technical limitations of Lightshow.

/cc @50Wliu, @mariozaizar

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM! I'm always in favor of switching for grammars that have tests!

@Alhadis
Copy link
Collaborator Author

Alhadis commented Sep 25, 2018

Tests are great until you need to update 300+ assertions because a scope was added. 😢

Not to mention they're so painful to write, I actually wrote an Atom command to generate them from a tokenised buffer's contents. Which had the oh-so-clever name of "Spec-saver". 👅

@Alhadis
Copy link
Collaborator Author

Alhadis commented Sep 25, 2018

Build is failing because of another license issue:

Warnings:
/home/travis/build/github/linguist/vendor/licenses/grammar/language-html.txt:
  - license needs reviewed: other.
310 dependencies checked, 1 warnings found.
The command "script/licensed status" exited with 1.

Tests passed locally though, so I don't know what's up with that...

@lildude
Copy link
Member

lildude commented Sep 25, 2018

Tests passed locally though, so I don't know what's up with that...

The caching of the license hasn't detected the license is MIT so has set it to "other" when caching. The script/licensed status test is picking this up.

We know this is MIT so feel free to set it manually in the license file.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Sep 25, 2018

Copyright (c) 2014 GitHub Inc.

GitHub staff approving the use of GitHub-licensed software for use on GitHub.

Are you sure that's safe?

@lildude
Copy link
Member

lildude commented Sep 25, 2018

Are you sure that's safe?

Not at all 😜

@50Wliu
Copy link

50Wliu commented Sep 25, 2018

No objections here!

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

LGTM

@lildude lildude merged commit 2aa5f0b into master Sep 25, 2018
@lildude lildude deleted the replace-html-grammar branch September 25, 2018 14:33
@vmg vmg mentioned this pull request Nov 12, 2018
17 tasks
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.

None yet

4 participants