capture: some initial cleanups #7261
Conversation
Removing this indirection enables some further clean ups.
With straight code, it is a little easier to understand, and simplify further.
This is more straightforward and does not require duplicating the initialization logic.
|
There is some problem on Windows. I'll try to figure it out somehow. |
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
nice refactoring, its indeed a bit big to get nicely in a single reading, i will take another look later today to gauge the details better
overall it looks good, thanks
| def __init__(self, in_, out, err) -> None: | ||
| self.in_ = in_ | ||
| self.out = out | ||
| self.err = err |
There was a problem hiding this comment.
its great this logic is gone from the ctor, an we move the outside factory functions to a named constructor
to encapsulate the in/out/err logic
also it looks to me if attrs couldbe used to declare the ctor/non-ctor atributes now
There was a problem hiding this comment.
attrs might be nice for some classes here. I'll try to remember this for later.
| def start(self): | ||
| setattr(sys, self.name, self.tmpfile) | ||
| self._state = "started" | ||
| def set_fixture(self, capture_fixture: "CaptureFixture") -> None: |
There was a problem hiding this comment.
at first glance i believe that pair might yield nicer code if it was a context manager instead
There was a problem hiding this comment.
So, it actually was a context manager, but the commits removes it. The reason is that the context manager adds some indirection which makes it more difficult to refactor further (in later PRs).
testing/test_capture.py
Outdated
| @pytest.mark.parametrize("use", [True, False]) | ||
| def test_fdcapture_tmpfile_remains_the_same(tmpfile, use): | ||
| if not use: | ||
| tmpfile = True |
There was a problem hiding this comment.
this test looks strange, whats the value of tmpfile?
There was a problem hiding this comment.
Right, I'm not sure what this was supposed to do either (it's the same in master -- the two parametrization are actually testing the same thing). I added a commit which removes the parametrization.
Make it easier to read the file in progression, and avoid forward references for upcoming type annotations. There is one cycle, CaptureManager <-> CaptureFixture, which is hard to untangle. (This commit should be added to `.gitblameignore`).
Since tmpfile is a parameter to SysCapture, it shouldn't assume things unnecessarily, when there is an alternative.
There are state transitions start/done/suspend/resume and two additional operations snap/writeorg. Previously it was not well defined in what order they can be called, and which operations are idempotent. Formalize this and enforce using assert checks with informative error messages if they fail (rather than random AttributeErrors).
This attribute is set in __init__ and not deleted. Other methods do it already but this one wasn't updated.
The two cases end up doing the same (the tmpfile fixture isn't used except being truthy).
|
The Windows failures are caused by commit "capture: fix invalid inheritance between Capture classes". The commit somehow changes the newlines-mode such that on I've removed the commit now, will resubmit once I have a chance to fix it up. |
bluetech
force-pushed
the
capture-cleanup-1
branch
from
8ba634a to
f93e021
Compare
2 participants
These are some simple cleanups to capture. I have more complex changes planned, but will send them separately.
The changes are split into independent commits. One of the commits reorders sections which makes the PR diff not very readable.