_GSConverter: handle stray 'GS' in output gracefully #20473
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
| raise ImageComparisonFailure( | ||
| (err + b"GS" + stack + b">") | ||
| .decode(sys.getfilesystemencoding(), "replace")) | ||
| err = self._read_until((b"GS<", b"GS>")) |
There was a problem hiding this comment.
AFAICT _read_until doesn't support tuples as is (len(terminator) is wrong) and needs to be fixed to support them.
There was a problem hiding this comment.
You are correct, I was so happy about figuring out that .endswith() accepts tuples that I've missed it ;-).
There was a problem hiding this comment.
Ok, so I've figured out that if instead of passing an explicit tuple, I use *terminator arg, I can allow multiple terminators without having to actually change the call sites or having to add a compatibility hack. If you don't like that, I can revert to adding if not isinstance(terminator, tuple) or changing all the call sites.
There was a problem hiding this comment.
Heh, wrong. I've just realized that stripping actually makes the err.endswith() test wrong :-/.
There was a problem hiding this comment.
I guess you need a read_until(*terminator, include_terminator=False|True)? Or just always include the terminator (returning a tuple for simpler use) and change all the call sites, perhaps that's slightly nicer APIwise.
There was a problem hiding this comment.
Ok, I think it's ready now. I've noticed another late-night mistake, and fixed it. I've aimed for minimal change / keeping the code simple now.
There was a problem hiding this comment.
I wonder if the whole thing would be simpler by having terminator be a regex instead, and return match.groups()? This way you can just match r"GS(?:<(\d+))>" instead of having to do two read_untils. (The slight extra cost of doing regex matches should be negligible compared to interacting with the subprocess.)
(If doing so, I would suggest having the argument be always a compiled regex object so that callers have to call re.compile themselves to make it completely clear that the argument is a regex.)
There was a problem hiding this comment.
To be honest, I don't really want to spend more time on it and I don't think it's really important how it's implemented given it's only used in tests.
b605a5b to
ff2b05c
Compare
Search the GS output stream for either "GS<" or "GS>" explicitly rather
than any "GS", in order to prevent the code from wrongly matching stray
"GS". This fixes a recent test regression on Gentoo where the following
output seems to have been wrongly matched:
**** Error 'gs' ignored -- ExtGState missing from Resources.
^^
Fixes matplotlib#20472
|
Thanks @mgorny ! Congratulations on your first Matplotlib PR merged 🎉 . Hopefully we will hear from you again. |
…473-on-v3.4.x Backport PR #20473 on branch v3.4.x (_GSConverter: handle stray 'GS' in output gracefully)
PR Summary
Search the GS output stream for either "GS<" or "GS>" explicitly rather
than any "GS", in order to prevent the code from wrongly matching stray
"GS". This fixes a recent test regression on Gentoo where the following
output seems to have been wrongly matched:
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).Fixes #20472