-
Notifications
You must be signed in to change notification settings - Fork 3
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
WIP: Base types #76
base: master
Are you sure you want to change the base?
WIP: Base types #76
Conversation
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
==========================================
- Coverage 100% 98.17% -1.83%
==========================================
Files 3 6 +3
Lines 101 164 +63
==========================================
+ Hits 101 161 +60
- Misses 0 3 +3
Continue to review full report at Codecov.
|
src/stdio_mgr/types.py
Outdated
from contextlib import AbstractContextManager | ||
except ImportError: # pragma: no cover | ||
|
||
class AbstractContextManager(abc.ABC): |
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.
this is needed because now all of the __enter__
/__exit__
use super()
so that changes in the MRO always have the desired effect, and object
doent have __enter__
/__exit__
.
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.
I need to look at this further, to understand it, before merging -- doing just a quick partial review for now
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.
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.
c.f. jazzband/contextlib2#21 for the backport of AbstractContextManager specifically. I suspect I can get a competent review there quite quickly by poking the right people.
I definitely like the use of inheritance to segregate/atomize the different units of functionality. But, the scope of the different pieces of functionality is such that I'm having a hard time keeping track of it when just looking at the source file. I feel like we should curate a diagrammatical inheritance tree of all this. Or, maybe there's already a tool out there that could autogenerate one into the docs??? (Would have to finish standing things up on the RTD side, in #35, so that we'd always have the most recent version available.) |
I was actually thinking about going to multiple source files. It looks like this PR encompasses both the refactor to the new If so, could you split this into two PRs, one for the refactor and one for the changes? Hopefully it should be easy to create and merge the no-change refactor as a new PR, and then rebase this one on top of it? |
The second of the three commits (1b6da37) includes a fix, but it is a fix for a bug that doesnt really exist yet, because stdio-mgr doesnt try to use the pre-existing streams yet. However it is a fix I need for nearly all paths forward. I'm happy to move it out to another commit as part of getting this one ready for merging. The first and third commits should have no functional changes. |
20be80c
to
b2996d5
Compare
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.
Partial review - I still need to look closely at the new(ish) classes.
Couple of changes, couple of questions.
src/stdio_mgr/types.py
Outdated
from contextlib import AbstractContextManager | ||
except ImportError: # pragma: no cover | ||
|
||
class AbstractContextManager(abc.ABC): |
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.
I need to look at this further, to understand it, before merging -- doing just a quick partial review for now
http://www.github.com/bskinn/stdio-mgr | ||
|
||
**Documentation** | ||
See README.rst at the GitHub repository |
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.
I was actually thinking about going to multiple source files.
I've been thinking about it for a while also, and types.py
isnt enough.
Possibly
iotypes.py
orio.py
for classes that do IO or areIOBase
subclasses, and- maybe
winio.py
for any Windows specific logic we may accumulate, and sys.py
and/orconsoleio.py
for any logic around detecting python's console state (e.g. buffered/unbuffered detection),stdiotypes.py
orstdio.py
for the tuple context-manager class hierarchy of re-usable components, leavingstdio_mgr.py
for finalised classes that implement high-level end-user-consumable tools.
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.
Yeah, that sort of structure makes sense to me.
sys.py
and/orconsoleio.py
for any logic around detecting python's console state (e.g. buffered/unbuffered detection),
Also in here any logic for dealing with cases when the sys.stdfoo
have already been wrapped/mocked by something else?
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.
I have stayed with types.py
for the tuple context-manager class hierarchy, as this is the core type hierarchy. Many of them are not strictly for tuples, or for context managers, or for stdio ;-).
Thoughts about using triples
in module / class names?
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.
Thoughts about using
triples
in module / class names?
As in Triple
would be the specific sub-case of Tuple
where it contains three items? Definitely good with that.
Separately, as I pore over these PRs, the value of #60 is becoming steadily clearer. I'm on board to fully type the thing.
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.
I started this change, but running out of time so havent finished addressing PR comments.
63397b5
to
9446d6e
Compare
|
||
mro = cm.__class__.__mro__ | ||
|
||
assert mro == ( |
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.
I hope this will help understanding. It could be annotated with comments of the methods inherited along the way.
And I think there should be similar for each new class that has any significant inheritance complexity.
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.
Yeah, and we could set the docs up with individual, class-specific inheritance diagrams for each one ... *sigh*, and, ideally there would be a way to automatically generate a diagram with the method inheritance cascade in the docs, so that we don't have to curate it manually.
@@ -0,0 +1,65 @@ | |||
r"""``stdio_mgr.compat`` *code module*. |
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.
could also be called backports
, and possibly _backports
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.
We might want both backports
and compat
... backports
would be for things like the AbstractContextManager
, whereas compat
would be helper stuff for coping with pytest, colorama, readline, etc.?
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.
Quite a bit to go over here. All in all, definitely liking the types/inheritance structure that's developing, I think it makes sense. I can put some thought into the machinery involved in exposing the overall class inheritance and the method overriding dynamics in an automated and intuitive way.
http://www.github.com/bskinn/stdio-mgr | ||
|
||
**Documentation** | ||
See README.rst at the GitHub repository |
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.
These documentation links in all source files should point to the new RtD docs: https://stdio-mgr.readthedocs.io/
src/stdio_mgr/types.py
Outdated
return self | ||
|
||
|
||
class FakeIOTuple(StdioTupleBase): |
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.
Eventually these will interact with ClosingStdioTuple
somewhere?
How do you envision ClosingStdioTuple
interacting with, e.g., _MultiCloseContextManager
?
src/stdio_mgr/types.py
Outdated
"""Tuple context manager of stdin, stdout and stderr-like objects.""" | ||
|
||
|
||
class AnyIOTuple(StdioTupleBase): |
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.
So...some of the items
in a FakeIOTuple
might actually be subclasses of _ITEM_BASE
, but the point of FakeIOTuple
is to not make any typed promises?
Whereas TextIOTuple
positively declares all of its items
as being subclasses of _ITEM_BASE
?
Beyond that, I'm not too much a fan of the name AnyIOTuple
... it doesn't really reflect how it automatically chooses the type of the return object based on the types of the items
. Something like AutoIOTuple
seems better.
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.
AutoIOTuple
is good.
the point of
FakeIOTuple
is to not make any typed promises?
yup. FakeIOTuple
would likely hold the algorithms for managing streams which are dubious, as they are not an io.TextIOBase
. We could go one step further and create a class for when all members are io.IOBase
, before falling back to a name/contract which implies the stream objects are potentially broken.
The one problem with this approach is that often a library doing wrapper/capture will stuff an poorly designed/constructed stream object into sys.stdin
while building decent classes for sys.stdout/err
, because they are only interested in the latter.
So there may not be too much benefit in defining "triple" varieties, as there are too many of them.
I expect we would create classes for WindowsUnbufferedConsole
and UnixUnbufferedConsole
, etc, if there are noticable implementation details and it is detectable (which it looks like you've solved in the other issue), and they are very common. Maybe even a PytestFdCapture
, Pytest4SysCapture
, Pytest5SysCapture
, etc if they have noticable differences between versions.
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.
Would there be value to a GenericIOTuple
, which promises some certain core set of "real IO" members on subclasses, that's either in addition to or instead of (probably in addition to) the FakeIOTuple
, which may contain items
that don't even provide what's needed for basic IO?
I guess IOTuple
might suffice; the Generic
might be superfluous.
This could be YAGNI at this point, though.
src/stdio_mgr/types.py
Outdated
def __new__(cls, iterable): | ||
"""Instantiate new TextIOTuple or FakeIOTuple from iterable.""" | ||
items = list(iterable) | ||
if any(not isinstance(item, TextIOTuple._ITEM_BASE) for item in items): |
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.
If the type check in TextIOTuple.__new__()
were stronger, this could just be:
try:
return TextIOTuple(items)
except {{{RelevantError}}}:
return FakeIOTuple(items)
tests/test_types.py
Outdated
assert str(err.value) == "underlying buffer has been detached" | ||
|
||
|
||
class _SafeCloseIOTuple(ClosingStdioTuple): |
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 is this defined de novo here? Is it just temporary, until it becomes sensible to move it to the actual codebase?
tests/test_types.py
Outdated
assert str(err.value) == "'str' object has no attribute 'close'" | ||
|
||
# The base safe close also doesnt handle closing incorrect types, | ||
# as it only valid types that become unusable, raising ValueError |
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.
Clarify/typo-check comment, pls
Please go ahead and rebase this onto It should make it a lot easier to put something together to "auto-update" the RTD inheritance diagram. |
Creates compat.py with backport of contextlib.AbstractContextManager to Python 3.4, so that the external interface of StdioManager has a consistent MRO irrespective of the Python version.
1f3f8fa
to
376c194
Compare
I think we can use doctests to monitor/report the method inheritance on each class. Hope to have a POC by tonight or tomorrow US Eastern. |
180cd54
to
ee9edda
Compare
IOTriple combines MultiItemIterable and TupleContextManager, and provides properties `stdin`, `stdout` and `stderr` for literate referencing the three members of the tuple. MultiItemIterable provides _map(), _all() and _any() for iterables, with suppress versions able to ignore exceptions. Base TupleContextManager creates a consistent MRO of tuple and AbstractContextManager. ClosingStdioTuple is a sample context manager which performs close() on each item in the tuple. TextIOTriple requires items to be a TextIOBase. AutoIOTriple can be used to create either a TextIOTriple or a FakeIOTriple, depending on whether the items are all a TextIOBase. This will allow attaching different implementations to each. AutoIOTuple is used to capture `sys.__std*__` and `sys.std*` as they existed at module import.
POC here: https://stdio-mgr.readthedocs.io/en/method-list/method_inheritance.html PR is #93 Ahhh, doctest is the wrong tool. What I want is something more like sphinx-autorun. |
@jayvdb Machinery is in place to auto-update Do you want me to add you to the email notifications, when the |
@jayvdb I think this PR is nearing a point where it could be merged. I'm waiting until it's merged to work on anything else significant in the package internals -- if you agree it's nearly there, let's make the push to get it finalized and merged? |
Sorry - been a bit busy elsewhere. Also, do you know what is causing the CI problem atm. I couldnt quickly spot the problem. |
<nod>, oh, no prob at all -- I've been splitting my time onto other stuff too. Just wanted to update you on why I haven't moved on anything here. And sure, I'll take a look @ CI, see what I can see. |
Ah, that makes sense. The Will have to do this for pretty much any new module. Once a v2.0 release is close, I'll split them out into separate docs pages as part of #35/#42/etc. |
Thanks. I'll try to get an update done over the weekend. |
This is the foundation of #64 , without any features in it, as the same foundation is also needed for #75
Also it partially resolves the question of whether to requires the previous streams are
TextIOBase
or not, essentially deciding that we need to accept that the code needs to allow for pooly implemented fake streams.It will fail coverage in small places.