-
Notifications
You must be signed in to change notification settings - Fork 111
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.
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
Convert GeneratorMixIn and AssemblingDecoderMixIn into proper MixIn-classes #7503
base: maint
Are you sure you want to change the base?
Convert GeneratorMixIn and AssemblingDecoderMixIn into proper MixIn-classes #7503
Conversation
This commit modifies `datalad.runner.protocol.GeneratorMixIn` to behave like a true Python-Mixin class. That means, it does not have a constructor anymore.
This commit removes constructors from most test protocols that use the generator mixin
This commit rmmoves the __init__-method from `datalad.runner.utils.AssemblingDecoderMixIn`, and adapts the code that uses it.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## maint #7503 +/- ##
===========================================
- Coverage 91.51% 51.22% -40.30%
===========================================
Files 325 325
Lines 43443 43393 -50
Branches 5827 0 -5827
===========================================
- Hits 39759 22226 -17533
- Misses 3669 21167 +17498
+ Partials 15 0 -15 ☔ View full report in Codecov by Sentry. |
Can #7346 be closed now? Probably worth mentioning here that ChatGPT already approved the removal of |
This commit removes the __init__-function from GeneratorMixIn, converting it into a real mix in class
f7c4eb6
to
e9dd361
Compare
I merged the code from PR #7346 and closed it. Waiting for CI to turn (mostly) green. |
for my own education -- do you have a URL for that Python documentation? |
@@ -146,14 +146,13 @@ def _readline_rstripped(stdout): | |||
return stdout.readline().rstrip() | |||
|
|||
|
|||
class BatchedCommandProtocol(GeneratorMixIn, StdOutErrCapture): | |||
class BatchedCommandProtocol(StdOutErrCapture, GeneratorMixIn): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why swap of the order in all (?) cases is needed?
nothing in the changelog entry hints on that (talks only about removal of __init__
) and brief descriptions of the commits (now 11) neither.
@@ -0,0 +1,4 @@ | |||
### 🏠 Internal | |||
|
|||
- Remove `__init__()` from `GeneratorMixIn` and `AssemblingDecoderMixIn` to convert them to "proper" mixin-classes. [PR #7346](https://github.com/datalad/datalad/pull/7346) (by [@christian-monch](https://github.com/christian-monch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove `__init__()` from `GeneratorMixIn` and `AssemblingDecoderMixIn` to convert them to "proper" mixin-classes. [PR #7346](https://github.com/datalad/datalad/pull/7346) (by [@christian-monch](https://github.com/christian-monch)) | |
- Remove `__init__()` from `GeneratorMixIn` and `AssemblingDecoderMixIn` to convert them to "proper" mixin-classes, and change order of inheritance to include mixin's last. [PR #7346](https://github.com/datalad/datalad/pull/7346) (by [@christian-monch](https://github.com/christian-monch)) |
for completeness (re comment below)
@@ -582,7 +563,9 @@ def timeout(self, fd: Optional[int]) -> bool: | |||
def test_exit_3() -> None: | |||
# Expect the process to be closed after | |||
# the generator exits. | |||
rt = ThreadedRunner(cmd=["sleep", "4"], | |||
rt = ThreadedRunner(cmd=["waitfor", "/T", "4", "TheComputerTurnsIntoATulip"] | |||
if on_windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: seems to not relate to mixin's RFing but since just in the tests, I guess it is ok to not bother describing in the changelog
def result_queue(self) -> deque: | ||
if not hasattr(self, '_result_queue'): | ||
self._result_queue: deque = deque() | ||
return self._result_queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since now result_queue
is not just an attribute and thus costs more upon each reuse, I think it is worth right away provide a local binding to a variable within _locked_send
function which has 7 references to this property.
@christian-monch will you have time to go through the comments? |
@christian-monch ping |
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
This PR changes the classes
GeneratorMixIn
andAssemblingDecoderMixIn
, which indicate by their name that they are "MixIn"-classes. The Python documentation states that "MixIn"-classes do not possess constructors.Up until now
GeneratorMixIn
andAssemblingDecoderMixIn
did possess a constructor, that had to be invoked for them to work properly. The changes in this PR remove the constructors and convertGeneratorMixIn
andAssemblingDecoderMixIn
into real MixIn-classes, i.e. they can be put in the list of parent classes and do not require further modifications of the defined class.