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

Add GraphQL #3448

Merged
merged 3 commits into from Feb 15, 2022
Merged

Add GraphQL #3448

merged 3 commits into from Feb 15, 2022

Conversation

jf990
Copy link
Contributor

@jf990 jf990 commented Dec 31, 2021

Add grammar GraphQL

Changes

languages/graphql.js, detect and markup tests

Checklist

  • Added markup tests
  • Updated the changelog at CHANGES.md

Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Lots of tiny things, but overall looks good - shouldn't be super hard to finish up.

src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

I plan to push a release in next few days just heads up, if this is ready it'll land in that.

@jf990
Copy link
Contributor Author

jf990 commented Jan 5, 2022

@joshgoebel suggestions applied and tested, looks good to me. thanks for your review.

@joshgoebel
Copy link
Member

Ping.

@joshgoebel joshgoebel marked this pull request as draft January 29, 2022 20:21
@jf990 jf990 marked this pull request as ready for review January 31, 2022 16:32
@jf990 jf990 requested a review from joshgoebel January 31, 2022 16:32
Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Can't tell what you changed, I'm not seeing changes on the comments I posted...?

src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
@jf990
Copy link
Contributor Author

jf990 commented Feb 2, 2022

@joshgoebel not sure how I missed your comments but I did, looking at it now.

@jf990 jf990 requested a review from joshgoebel February 2, 2022 22:58
src/languages/graphql.js Outdated Show resolved Hide resolved
src/languages/graphql.js Outdated Show resolved Hide resolved
Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Almost there. :-) Thanks for all the effort.

@joshgoebel
Copy link
Member

Long term (if we consider this for adding to default) we might need to add some combo rules such as:

mutation [IDENT_RE] {... and of course the mutation name would also be a title.class, etc... So then that would only match mutation Book { but not mutation { from some other language... and then we could perhaps bump the relevance for those keywords (in context) a little higher... but I think this is a good start for now.

@joshgoebel
Copy link
Member

Cool, I'll let this simmer another day or two then circle back for a final review. I find that often catches things I miss, but I I can probably fix up anything else I find. Thanks for all the help!

@joshgoebel joshgoebel marked this pull request as draft February 9, 2022 20:15
@jf990 jf990 marked this pull request as ready for review February 15, 2022 17:24
@joshgoebel
Copy link
Member

@jf990 Thanks for all the work on this!

@joshgoebel
Copy link
Member

Size Change: +1.15 kB (0%)
Total Size: 715 kB

Filename Size Change
./build/es/languages/stata.min.js 6.65 kB +10 B (0%)
./build/languages/pf.min.js 849 B +6 B (+1%)
./build/languages/stata.min.js 6.66 kB +13 B (0%)
./build/es/languages/graphql.min.js 543 B +543 B (new file) 🆕
./build/languages/graphql.min.js 550 B +550 B (new file) 🆕

@joshgoebel joshgoebel merged commit 8aef7b6 into highlightjs:main Feb 15, 2022
@jf990
Copy link
Contributor Author

jf990 commented Feb 15, 2022

@joshgoebel thanks for your help, definitely much better thanks to your review feedback.

@jf990 jf990 deleted the add-graphql branch February 15, 2022 19:31
@eric-wieser eric-wieser mentioned this pull request Jun 4, 2022
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

2 participants