-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support multiline graphql descriptions (Fix #2405) #2406
Support multiline graphql descriptions (Fix #2405) #2406
Conversation
Thank you @AlexMabry for this PR! To make the CI pass, you have to rebuild Prism using the command Before we merge this, we also have to talk about whether we want Markdown highlighting inside multiline strings. You added this because they are frequently used for this purpose (I assume) but 1) single-line strings may also be used for descriptions and 2) multi-line strings may not contain Markdown if used as values. Here is my suggestion on how we could do this: 'description': {
pattern: /(?:"""(?:[^"]|(?!""")")*"""|"(?:\\.|[^\\"\r\n])*")(?=\s*[a-z_])/i,
greedy: true,
alias: 'string',
inside: {
// TODO: Exclude leading and trailing "
'language-markdown': {
pattern: /[\s\S]+/,
inside: Prism.languages.markdown
}
}
},
'string': {
pattern: /"""(?:[^"]|(?!""")")*"""|"(?:\\.|[^\\"\r\n])*"/,
greedy: true
}, The basic idea is that a description is always followed by either a keyword or a name (see grammar), so the Thoughts? (I also had to slightly change the multi-line string pattern because of the lookahead (see lazy trap).) |
Also, if Markdown is optional, I think it needs to be added to the language conditionally. |
@mAAdhaTTah I think it's totally fine to have a |
@RunDevelopment Thanks for the suggestions. I agree with you that descriptions and strings should be treated differently. After making your change, I also made the additional changes:
It looks likes CI is still failing, so I will look into that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes @AlexMabry.
Tests and examples look good but two more things.
First, string
vs comment
. I can see why you changed it to comment
. Most popular languages use comments for documentation (e.g. C++, Java, JS) while only a few use strings (e.g. Python).
However, I think it should still be highlighted as string
. The spec and many articles I found refer to them as descript (or documentation) strings. Since people seem to think about them as strings (similar to Python), I think we should also highlight them as such.
Second, please see the below comment on leading and trailing quotes.
components/prism-graphql.js
Outdated
// TODO: Exclude leading and trailing " | ||
'language-markdown': { | ||
pattern: /[\s\S]+/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the only remaining question is: How do we handle this?
I want to exclude the leading and trailing quotes because some of Markdown's features are line-based and therefore might consume the trailing quotes.
I think that this solution is kinda dumb but it'll work:
// TODO: Exclude leading and trailing " | |
'language-markdown': { | |
pattern: /[\s\S]+/, | |
'language-markdown': { | |
pattern: /(^"(?:"")?)(?!\1)[\s\S]+(?=\1$)/, | |
lookbehind: true, |
The basic idea is to use a backreference to exclude the same number of leading and trailing quotes. The (?!\1)
lookahead will then ensure that an empty multi-line string will not have a language-markdown
token.
This is the only correct solution I can think of but it's not obvious or elegant. Does anybody have a simpler solution for this?
I just made the changes that you recommended. |
Thank you very much for contributing @AlexMabry! |
I took a stab at this, hoping to save you some time.