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

Switch out JSON highlighter #5507

Merged
merged 4 commits into from
Sep 2, 2021
Merged

Switch out JSON highlighter #5507

merged 4 commits into from
Sep 2, 2021

Conversation

Nixinova
Copy link
Contributor

@Nixinova Nixinova commented Aug 7, 2021

Description

JSON files are currently quite difficult to read, as keys and strings are all coloured the same:

It would be much easier to read JSON files if the keys were coloured differently:

This changes the JSON grammar to one that highlights keys separately from strings, making JSON files must easier to read.

Checklist

Languages affected

  • Ecere Projects
  • JSON
  • (not) Jupyter Notebook
  • Max

Change to one that highlights keys separately from strings, making JSON files must easier to read.
@Nixinova Nixinova requested a review from a team as a code owner August 7, 2021 22:03
@Nixinova
Copy link
Contributor Author

Nixinova commented Aug 8, 2021

Getting a contentless error

@lildude
Copy link
Member

lildude commented Aug 9, 2021

Getting a contentless error

No you're not. Here's your error: https://github.com/github/linguist/pull/5507/checks?check_run_id=3271010550#step:4:704

@lildude
Copy link
Member

lildude commented Aug 9, 2021

🤔 I'm not sure this is the right thing to do. Thoughts @Alhadis as you're more experienced with writing grammars.

What's more, peeps aren't going to notice when using light mode as you can barely tell the difference between the two when viewing on lightshow

@Nixinova
Copy link
Contributor Author

I'm not sure this is the right thing to do.

People really don't like the current syntax highlighting and neither do I. I can try to find a better scope name though if they look too identical on light mode (though who uses that slight /s).

@Alhadis
Copy link
Collaborator

Alhadis commented Aug 10, 2021

What's more, peeps aren't going to notice when using light mode

It might be because I'm colour-blind, but I don't see much difference in the dark-mode highlighting, either.

Regarding the grammar, there's a tonne of mistakes and missing syntax that are too numerous to list individually, so I've sent you a pull-request instead.

(though who uses that slight /s).

I do. 😜

@lildude
Copy link
Member

lildude commented Aug 26, 2021

🤔 this appears to have a wider ranging effect as other languages use the JSON grammar. How do your changes affect those?

Could you add before and after links for each in the OP for easy comparison.

@Nixinova
Copy link
Contributor Author

How do your changes affect those?

Well if they're using the JSON grammar then they're normal json and should be fine.

@Nixinova
Copy link
Contributor Author

Ecere Projects however (doesn't have a sample here) & isn't json anyway though so unsure why it has the tm_scope (and why it's a language in the first place)...

@Alhadis
Copy link
Collaborator

Alhadis commented Aug 27, 2021

Well if they're using the JSON grammar then they're normal json and should be fine.

That's not entirely true: grammars can import specific rules from other grammars using the form

include: "source.json#rule-name"

… which is something I liberally abuse the shit out of with my etc "grammar", e.g.:

include: "etc#str"

@lildude
Copy link
Member

lildude commented Sep 2, 2021

  • (not) Jupyter Notebook

Not not Jupyter Notebook 😁. The underlying raw source looks like JSON to me. It looks good with your grammar, as do the others affected.

Ecere Projects however (doesn't have a sample here) & isn't json anyway though so unsure why it has the tm_scope (and why it's a language in the first place)...

I can't answer why it's a language, but from a quick search of projects with .ejp extensions, they do mostly appear to be JSON-like. They also look good with your grammar too. You seemed to have struck lucky with your find.

That's not entirely true: grammars can import specific rules from other grammars using the form

include: "source.json#rule-name"

Looks like none of the grammars we're using is doing this for the JSON grammar 🎉 .

So all in, I think we're good to switch out the JSON grammar.

@lildude lildude merged commit 13adea7 into github-linguist:master Sep 2, 2021
@Nixinova Nixinova deleted the switch-json-grammar branch September 2, 2021 21:23
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

3 participants