8000 _GSConverter: handle stray 'GS' in output gracefully by mgorny · Pull Request #20473 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

_GSConverter: handle stray 'GS' in output gracefully #20473

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 21, 2021
Merged

_GSConverter: handle stray 'GS' in output gracefully #20473

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions lib/matplotlib/testing/compare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def _read_until(self, terminator):
raise _ConverterError
buf.extend(c)
if buf.endswith(terminator):
return bytes(buf[:-len(terminator)])
return bytes(buf)


class _GSConverter(_Converter):
Expand Down Expand Up @@ -154,15 +154,16 @@ def encode_and_escape(name):
+ b") run flush\n")
self._proc.stdin.flush()
# GS> if nothing left on the stack; GS<n> if n items left on the stack.
err = self._read_until(b"GS")
stack = self._read_until(b">")
err = self._read_until((b"GS<", b"GS>"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT _read_until doesn't support tuples as is (len(terminator) is wrong) and needs to be fixed to support them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I was so happy about figuring out that .endswith() accepts tuples that I've missed it ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, wrong. I've just realized that stripping actually makes the err.endswith() test wrong :-/.

Copy link
Contributor
@anntzer anntzer Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

stack = ""
if err.endswith(b"GS<"):
stack = self._read_until(b">")
if stack or not os.path.exists(dest):
stack_size = int(stack[1:]) if stack else 0
stack_size = int(stack[:-1]) if stack else 0
self._proc.stdin.write(b"pop\n" * stack_size)
# Using the systemencoding should at least get the filenames right.
raise ImageComparisonFailure(
(err + b"GS" + stack + b">")
.decode(sys.getfilesystemencoding(), "replace"))
(err + stack).decode(sys.getfilesystemencoding(), "replace"))


class _SVGConverter(_Converter):
Expand Down
0