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 CriticMarkup scope names #229

Merged
merged 1 commit into from May 9, 2018

Conversation

Projects
None yet
3 participants
@davidar
Contributor

davidar commented Apr 21, 2018

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

This PR changes the scope names for CriticMarkup insertions, deletions and substitutions to be more in line with existing naming conventions, in particular language-diff, the official GitHub syntax theme, and the TextMate naming conventions.

Alternate Designs

It might have been cleaner to change the scope names to something like markup.{inserted,deleted,changed}.gfm, but this would likely break existing themes relying on the current scope names. To avoid this, the current scope names have been retained as a suffix.

Benefits

The main benefit is better compatibility with existing syntax themes:

Before - GitHub Light syntax theme

After - GitHub Light syntax theme

Before - One Dark syntax theme

After - One Dark syntax theme

Possible Drawbacks

As noted earlier, changing the scope names runs the risk of breaking existing themes, but I've tried to avoid this from happening by not removing any of the groups currently in the scope names.

Before - Pen Paper Coffee syntax theme

After - Pen Paper Coffee syntax theme

Applicable Issues

github/linguist#4108

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented May 1, 2018

@50Wliu Should this be an injection grammar instead?

@50Wliu

This comment has been minimized.

Member

50Wliu commented May 5, 2018

It doesn't look like there already exists a language for CriticMarkup. I also don't know how well Linguist supports injection grammars. I'm fine with these scopes changing though since nothing got removed.

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented May 8, 2018

If you're 👍 on merging this @50Wliu, feel free or let me know if you'd rather I take care of it.

@50Wliu 50Wliu merged commit 36156a0 into atom:master May 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment