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

Remove invalid string highlighting from TextMate grammar #642

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
2 participants
@lildude
Copy link
Contributor

commented Mar 15, 2019

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

GitHub's Linguist, which is used for syntax highlighting on GitHub.com, currently uses this grammar for highlighting Javascript and JSX. For quite some time now, users have experienced the problem where the presence of an apostrophe within JSX causes the syntax to be highlighted in red as an error:

@Alhadis proposed the solution of removing the "rule which highlights unterminated strings as errors" which @50Wliu suggested would probably be fine as the TextMate grammars are no longer actively maintained:

@Alhadis that's probably fine. We don't actively maintain the TextMate variants anymore, but I think this is something we'll merge a PR for.

@50Wliu opened #641 to track this. I'm now implementing the changes suggested by @Alhadis in this PR.

This change only affects the TextMate grammar as this is the grammar Linguist uses as tree-sitter grammars are not currently supported.

Alternate Designs

Benefits

Syntax highlighting on GitHub.com for Javascript with JSX that contains apostrophes will be rendered correctly.

Possible Drawbacks

As this change only affects the TextMate grammar, there is a risk that the editor behaviour may vary slightly from the representation on GitHub.com for as long as we use the TextMate grammar.

Applicable Issues

See github/linguist#3044 for further details of the problem we're trying to resolve.

Fixes #641

Remove invalid string highlighting
GitHub Linguist currently uses this grammar for highlighting Javascript and JSX and the presence of an apostrophe within JSX caused the syntax to be highlighted in red. See github/linguist#3044 for details.

Fixes #641
@lildude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

🤔 Sorting out the tests is going to be interesting as we only want these changes to be considered when testing the TextMate grammar. I'm open to suggestions on how best to do this.

@lildude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Ooo, maybe not. If I'm reading the tests correctly, the tests only run against the TextMate grammar:

beforeEach ->
atom.config.set('core.useTreeSitterParsers', false)

So whatever test changes I make, they'll only apply to the testing with the TextMate grammar.

@50Wliu

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I thought about this for a bit: wouldn't this still make it so everything will be matched as a string until the next quote? I suppose that's better than angry red highlighting, but still not that ideal.

@lildude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I thought about this for a bit: wouldn't this still make it so everything will be matched as a string until the next quote?

Yup. I've gone back to @Alhadis to see if he's got a better idea or implementation method as I agree, it's not ideal, however it's certainly waaaay better than all the red... like in this PR https://github.com/peterbe/whatsdeployed/pull/81/files 😭

@lildude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Heard back:

I thought about this for a bit: wouldn't this still make it so everything will be matched as a string until the next quote?

Yup, but this would only affect GitHub and Atom users that have the treesitter grammars disabled. An alternate solution is waaay too complex and requires changes to Atom.

@50Wliu

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

All right, let's do this then.

@50Wliu 50Wliu merged commit 0a71325 into atom:master Apr 1, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

🚀 as language-javascript@0.129.21.

@lildude lildude deleted the lildude:remove-invalid-string-for-linguist branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.