Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Syntax highlight for graphql_ppx #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Huxpro
Copy link
Contributor

@Huxpro Huxpro commented Jun 18, 2018

Hey! This PR adds the syntax highlight for GraphQL_PPX!
The graphql grammar was shamelessly borrowed from graphql-for-vscode (or vscode-graphql, they're the same). So the color for GraphQL queries would be usually same with static *.graphql file. It actually did pretty bad in graphql variables tho. :(

The highlight for [%graphql] attributes and {||} are customized in my personal taste by using a monolithic string-ish color. So they won't be too poped out ;)

screen shot 2018-06-18 at 9 51 21 am

@ghost
Copy link

ghost commented Jun 20, 2018

Thanks for the PR!

I think it would be nice to merge this into the extension but I don't quite understand why we want to duplicate the graphql highlighting here rather than relying on the existing plugin for that (via "include"). Are you saying you wanted to do that just to customize the scopes for different colorization than the other one uses?

@Huxpro
Copy link
Contributor Author

Huxpro commented Jun 22, 2018

@freebroccolo I was thinking of adding it to graphql one as well and personally I had no opinion with either. But I found we actually got more than one graphql-plugin in the market 😅 (so we dont need to depend on one and ask user to install it)

Another advantage might be that it's easier to customize it for reason needs. (e.g. adding new pattern, make the highlight more compatible with reason).

@ghost
Copy link

ghost commented Jun 24, 2018

@Huxpro thanks for the clarification.

Honestly, I would prefer to avoid including the basic graphql highlighting in this plugin. I understand your point about the flexibility but it's going to mean more maintenance and it's something I don't use so that doesn't seem like an ideal combination. I'd also like to retain as much compatibility with other existing graphql extensions as possible.

I'd like to investigate the situation further by looking at the other extensions but it's going to be a couple of weeks before I have time to do it properly (currently very busy with work stuff). Just wanted to mention that so it doesn't seem like I'm ignoring this PR.

@zhangchen91
Copy link

jump into graphql?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants