Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix mathtext mismatched braces#26519
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
We discussed this a bit on GSOC call, the error message is not the most useful, but it is actually pretty hard to get a better one (the Current feeling is that erroring is better than not, but if someone has an idea of how to get the parser to give a more informative message, I'd be open to hearing it. |
Actually, this diff may get us a better error message: diff --git a/lib/matplotlib/_mathtext.py b/lib/matplotlib/_mathtext.pyindex 25a825b7b0..de4c25bfe5 100644--- a/lib/matplotlib/_mathtext.py+++ b/lib/matplotlib/_mathtext.py@@ -1894,6 +1894,7 @@ class Parser: p.function = csnames("name", self._function_names) p.group = p.start_group + ZeroOrMore(p.token)("group") + p.end_group+ p.unclosed_group = p.start_group + ZeroOrMore(p.token)("group") + StringEnd() p.frac = cmd(r"\frac", p.required_group("num") + p.required_group("den")) p.dfrac = cmd(r"\dfrac", p.required_group("num") + p.required_group("den"))@@ -1943,6 +1944,7 @@ class Parser: p.simple | p.auto_delim | p.unknown_symbol # Must be last+ | p.unclosed_group # Must be last ) p.operatorname = cmd(r"\operatorname", "{" + ZeroOrMore(p.simple)("name") + "}")@@ -2144,6 +2146,9 @@ class Parser: def unknown_symbol(self, s, loc, toks): raise ParseFatalException(s, loc, f"Unknown symbol: {toks['name']}")+ def unclosed_group(self, s, loc, toks):+ raise ParseFatalException(s, len(s), "Expected '}'")+ _accent_map = { r'hat': r'\circumflexaccent', r'breve': r'\combiningbreve',diff --git a/lib/matplotlib/tests/test_mathtext.py b/lib/matplotlib/tests/test_mathtext.pyindex d80312495d..5a9e8a8b92 100644--- a/lib/matplotlib/tests/test_mathtext.py+++ b/lib/matplotlib/tests/test_mathtext.py@@ -320,6 +320,7 @@ def test_fontinfo(): (r'$a^2^2$', r'Double superscript'), (r'$a_2_2$', r'Double subscript'), (r'$a^2_a^2$', r'Double superscript'),+ (r'$a = {b$', r"Expected '}'"), ], ids=[ 'hspace without value',@@ -347,7 +348,8 @@ def test_fontinfo(): 'unknown symbol', 'double superscript', 'double subscript',- 'super on sub without braces'+ 'super on sub without braces',+ 'unclosed group', ] ) def test_mathtext_exceptions(math, msg): @devRD thoughts? |
I think the diff makes sense, we do get a better error message. Added the changes... |
lib/matplotlib/_mathtext.py Outdated
| @@ -1865,6 +1865,8 @@ def csnames(group, names): | |||
| "|".join(map(re.escape, tex2uni))) | |||
| )("sym").leaveWhitespace() | |||
| p.unknown_symbol = Regex(r"\\[A-Za-z]+")("name") | |||
| def unclosed_group(self, s, loc, toks): | |||
| raise ParseFatalException(s, len(s), "Expected '}'") | |||
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.
The location of this method is not that logical. I'd suggest moving it down to other group-methods. Otherwise, it looks good!
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.
Let me rephrase that: I think this should be removed as there is another method below (at a different level, so I wonder if this is used at all?).
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.
I think the method is being utilized because we get the same error message as above.
Re:there is another method below (at a different level - I might be missing something, could you point out which one?
Although not completely sure if this is correct, I think the same behavior is also achieved by redefining the Instead, the error message here would be |
lib/matplotlib/_mathtext.py Outdated
| @@ -2144,6 +2149,9 @@ def symbol(self, s, loc, toks): | |||
| def unknown_symbol(self, s, loc, toks): | |||
| raise ParseFatalException(s, loc, f"Unknown symbol: {toks['name']}") | |||
| def unclosed_group(self, s, loc, toks): | |||
| raise ParseFatalException(s, len(s), "Expected '}'") | |||
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.
Here is the other, identical, method. Should only require one and I think this is the one.
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.
Yup, I missed that. Thanks!
Co-authored-by: Kyle Sunden <git@ksunden.space>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Looks good! Not sure if it should go into 3.8 or 3.9 (I'd vote, always, for 3.8...), so I let one of the reviewers closer to the release process tag and merge.
…519-on-v3.8.xBackport PR#26519 on branch v3.8.x (Fix mathtext mismatched braces)
PR summary
Fixes#26510
PR checklist