Repository navigation
Conversation
…icated function - _deduceGraphFormat
02d5ebd to
58de998
Compare
| return True | ||
| return super().saveGraph(filename=filename, fileFormat=fileFormat, dpi=dpi) | ||
| if fileFormat is None: | ||
| fileFormat = _deduceGraphFormat(filename) |
| if fileFormat is None: | ||
| fileFormat = _deduceGraphFormat(filename) | ||
| if fileFormat != "png": | ||
| return super().saveGraph(filename=filename, fileFormat=fileFormat, dpi=dpi) |
There was a problem hiding this comment.
AI replaces the f fileFormat == "png" by f fileFormat != "png". And I agree this is a bit simpler to read. So I let as it was.
| buffer = qt.QBuffer() | ||
| buffer.open(qt.QIODevice.WriteOnly) | ||
| try: | ||
| if not image.save(buffer, "png"): | ||
| return False | ||
| filename.write(bytes(buffer.data())) | ||
| finally: | ||
| buffer.close() | ||
| return True |
There was a problem hiding this comment.
This is the fix of the bug.
I didn't created a context manager because it seems simpler IMO. But can be discussed.
There was a problem hiding this comment.
Feels weird that we don't already have a util for this. I feel that we are doing similar things elsewhere.
| yield plot | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("fileFormat", ["png", None]) |
There was a problem hiding this comment.
The None part of those test are testing the calls to _deduceGraphFormat. Not clear to me if this is over-killed or not.
There was a problem hiding this comment.
I think it is good to test for None but it should be a specific test IMO
payno
changed the title
| self.assertEqual(self.plot.isKeepDataAspectRatio(), isKeepAspectRatio) | ||
|
|
||
|
|
||
| @pytest.fixture(params=[PlotWidget, PlotWindow]) |
There was a problem hiding this comment.
There is a specific test_plotwidget file so perhaps the test for PlotWidget should live there?
| yield plot | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("fileFormat", ["png", None]) |
There was a problem hiding this comment.
I think it is good to test for None but it should be a specific test IMO
|
|
||
|
|
||
| @pytest.mark.skipif(not qt.HAS_SVG, reason="QtSvg not available") | ||
| @pytest.mark.parametrize("fileFormat", ["svg", None]) |
There was a problem hiding this comment.
What happens if fileFormat is png here?
| """Test saveGraph fails when format cannot be deduced""" | ||
| assert not plotWithImage.saveGraph(BytesIO()) |
There was a problem hiding this comment.
Should it fail silenty or raise an error?
| """Returns the file format from the filename extension.""" | ||
| if not isinstance(filename, str): | ||
| return None | ||
| return filename.split(".")[-1].lower() |
There was a problem hiding this comment.
| return filename.split(".")[-1].lower() | |
| return filename.rsplit(".")[-1].lower() |
| buffer = qt.QBuffer() | ||
| buffer.open(qt.QIODevice.WriteOnly) | ||
| try: | ||
| if not image.save(buffer, "png"): | ||
| return False | ||
| filename.write(bytes(buffer.data())) | ||
| finally: | ||
| buffer.close() | ||
| return True |
There was a problem hiding this comment.
Feels weird that we don't already have a util for this. I feel that we are doing similar things elsewhere.
| if fileFormat is None: | ||
| fileFormat = _deduceGraphFormat(filename) | ||
| if fileFormat != "png": | ||
| return super().saveGraph(filename=filename, fileFormat=fileFormat, dpi=dpi) |
PR summary
close #4759 and also improve code by checking upstream the file format.
AI Disclosure