Conversation
| p.accent <<= Group( | ||
| Suppress(p.bslash) | ||
| + oneOf([*self._accent_map, *self._wide_accents]) | ||
| + Suppress(Optional(White())) |
There was a problem hiding this comment.
If the code is not working without this explicit whitespace-skipping, that is a pyparsing bug. Since you have made it Optional, then this won't break once the pyparsing bug is fixed. (You can also - or at least should be able to - use Empty() as a synonym for Suppress(Optional(White()))
There was a problem hiding this comment.
Ok. There is a bug in pyparsing, but fixing it doesn't completely solve your problem.
What is the purpose of leaveWhitespace() here?
p.symbol <<= (p.single_symbol | p.symbol_name).leaveWhitespace()
It looks like this is the only element in p.placeable that suppresses whitespace-skipping. The new code detects this and instead of doing whitespace-skipping before the MatchFirst, it doesn't do whitespace-skipping, relying on all the separate elements to do it themselves (this is the performance optimization - if all the alternatives will want whitespace to be skipped, the MatchFirst will do it once, and then all the alternatives get a pass). Since p.symbol suppresses whitespace-skipping, and so makes p.placeable suppress whitespace-skipping, then when it is time to match p.symbol, the leading space has not been skipped, and so it fails.
So:
-
I need to fix the bug in pyparsing - this will be in 3.0.4, which I will push out this evening.
-
This is another pyparsing improvement that needs to be documented in the "stuff that has changed for the better, but may affect your code" - I'll add that information in 3.0.4 also.
-
Please revisit why you need to have
leaveWhitespace()onp.symbol.If it is required, then I think you could remove the
Suppress(Optional(White()))fromp.accent, and instead change thep.symbolusage inp.placeableto:| Empty() + p.symbol # Must be third to catch all named symbols and single(I tested this in my minimal repro example and that works.)
But ideally, you could just remove the call toleaveWhitespace().
There was a problem hiding this comment.
Agreed that removing leaveWhitespace seems to work; looking at the git history it seems to be a historical artefact of the times when symbol and symbol_name were not split, and was used for symbol_name.
jklymak
left a comment
There was a problem hiding this comment.
@anntzer are you in agreement that this should go in as-is and be back ported per @tacaswell analysis in #21606?
|
Yes per #21606 (comment). |
…501-on-v3.5.x Backport PR #21501 on branch v3.5.x (Refix for pyparsing compat.)
We missed relaxing the requirement in matplotlib#21501
PR Summary
cf #21489 (comment) (@ptmcg I'm not super certain about the expected semantics, but the need to explicitly skip whitespace here looks like a bug to me?)
For master, I think #21448 also fixes that.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).