Conversation
jeanas
left a comment
jeanas
left a comment
There was a problem hiding this comment.
Thanks for preparing this. Here are some preliminary comments. I think you can simplify it a lot with the ("#pop", "other_state") principle.
| @@ -0,0 +1,205 @@ | |||
| from pygments.lexer import RegexLexer, words, include, bygroups | |||
There was a problem hiding this comment.
The file is missing a docstring with module info and copyright header. See other files for a template, and run make check to ensure it's correct (the CI won't pass without this).
| ) | ||
|
|
||
|
|
||
| class GraphQLLexer(RegexLexer): |
There was a problem hiding this comment.
You need to add a docstring, for consumption by https://pygments.org/docs/lexers/. Don't forget the .. versionadded:: 2.12 directive.
|
|
||
| tokens = { | ||
| "ignored_tokens": [ | ||
| (r"([ \t]|[\r\n]|\r\n)+", Text), # Whitespaces |
There was a problem hiding this comment.
Just \s+? Also, please use Whitespace instead of Text (see #1905).
| ), | ||
| (r"-?\d+", Number.Integer, "#pop"), | ||
| (r'"', String, "string0"), | ||
| (words(BOOLEAN_VALUES, suffix=r"\b"), Name.Builtin, "#pop"), |
There was a problem hiding this comment.
Does Keyword.Pseudo apply here? (I don't know the language.) See https://pygments.org/docs/tokens/ for a description.
| (r"-?\d+", Number.Integer, "#pop"), | ||
| (r'"', String, "string0"), | ||
| (words(BOOLEAN_VALUES, suffix=r"\b"), Name.Builtin, "#pop"), | ||
| (r"\$[a-zA-Z_]\w*", Name.Variable, "#pop"), |
There was a problem hiding this comment.
Is Unicode supported? If so, [^\W\d] would be better than [a-zA-Z_] (same thing in other places).
| (words(BOOLEAN_VALUES, suffix=r"\b"), Name.Builtin, "#pop"), | ||
| (r"\$[a-zA-Z_]\w*", Name.Variable, "#pop"), | ||
| (r"[a-zA-Z_]\w*", Name.Constant, "#pop"), | ||
| (r"\[", Punctuation, "list_value0"), |
There was a problem hiding this comment.
Instead of the include() dance between list_value0 and list_value, I think it would be simpler to do
(r"\[", Punctuation, ("#pop", "list_value")),
and keep just one list_value state. The same seems to apply to your remark above about state duplication.
| ], | ||
| "selection_set": [ | ||
| include("ignored_tokens"), | ||
| (r"([a-zA-Z_]\w*)(\s*)(:)", bygroups(Name.Label, Text, Punctuation)), |
| (r"[a-zA-Z_]\w*", Name), # Field | ||
| ( | ||
| r"(\.\.\.)(\s+)(on)\b", | ||
| bygroups(Punctuation, Text, Keyword), |
| (56, Punctuation, "}"), | ||
| (57, Text, "\n"), | ||
| ] | ||
| assert_tokens_equal(self, text, expected) |
There was a problem hiding this comment.
On this file: this is a lot of test code, and generally most of it can simply go as the other tests in tests/examplefiles/, or better yet, tests/snippets/ (which is a bit more practical for small test files; you could also move the other tests there). You could keep the tests that start on a specific state, but maybe there is a simple pattern that you can use to get in a value context, to keep everything in tests/snippets/?
|
Is this branch / feature dead / stale? Is the original PR opener planning on going through these changes? |
|
I think it's fair to say it's been a while, if you want to pick up the work from here, this would be great so we can eventually land GraphQL support. |
|
Closing in favor of #2428 |
4 participants
Closes #1634.