WIP: Base types #76

jayvdb · 2019-09-06T10:36:09Z

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.

codecov-io · 2019-09-06T10:45:55Z

Codecov Report

Merging #76 into master will decrease coverage by 1.82%.
The diff coverage is 96.29%.

@@            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

Impacted Files	Coverage Δ
src/stdio_mgr/compat.py	`0% <0%> (ø)`
src/stdio_mgr/triple.py	`100% <100%> (ø)`
src/stdio_mgr/types.py	`100% <100%> (ø)`
src/stdio_mgr/stdio_mgr.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 431059c...80f8b03. Read the comment docs.

src/stdio_mgr/types.py

jayvdb · 2019-09-06T15:13:31Z

src/stdio_mgr/types.py

+    from contextlib import AbstractContextManager
+except ImportError:  # pragma: no cover
+
+    class AbstractContextManager(abc.ABC):


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

I need to look at this further, to understand it, before merging -- doing just a quick partial review for now

It still has coverage problems, sort-of-intentionally, as I'd rather not have it merged until TextIOTuple is integrated or removed, and I'd like to have #78 or #64 also mostly complete. It is separate PR as it is common to both paths forward.

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.

bskinn · 2019-09-06T15:46:27Z

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

bskinn · 2019-09-06T16:03:35Z

I was actually thinking about going to multiple source files.

It looks like this PR encompasses both the refactor to the new types.py, and perhaps also some changes?

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?

jayvdb · 2019-09-06T16:28:50Z

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.

bskinn

Partial review - I still need to look closely at the new(ish) classes.

Couple of changes, couple of questions.

src/stdio_mgr/stdio_mgr.py

bskinn · 2019-09-08T02:40:35Z

src/stdio_mgr/types.py

+    from contextlib import AbstractContextManager
+except ImportError:  # pragma: no cover
+
+    class AbstractContextManager(abc.ABC):


I need to look at this further, to understand it, before merging -- doing just a quick partial review for now

tests/test_stdiomgr_base.py

src/stdio_mgr/types.py

jayvdb · 2019-09-08T08:19:22Z

src/stdio_mgr/types.py

+    http://www.github.com/bskinn/stdio-mgr
+
+**Documentation**
+    See README.rst at the GitHub repository


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 or io.py for classes that do IO or are IOBase subclasses, and

maybe winio.py for any Windows specific logic we may accumulate, and

sys.py and/or consoleio.py for any logic around detecting python's console state (e.g. buffered/unbuffered detection),

stdiotypes.py or stdio.py for the tuple context-manager class hierarchy of re-usable components, leaving

stdio_mgr.py for finalised classes that implement high-level end-user-consumable tools.

Yeah, that sort of structure makes sense to me.

sys.py and/or consoleio.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?

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?

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.

I started this change, but running out of time so havent finished addressing PR comments.

src/stdio_mgr/types.py

jayvdb · 2019-09-09T16:46:35Z

tests/test_stdiomgr_base.py

+
+    mro = cm.__class__.__mro__
+
+    assert mro == (


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.

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.

jayvdb · 2019-09-09T17:06:35Z

src/stdio_mgr/compat.py

@@ -0,0 +1,65 @@
+r"""``stdio_mgr.compat`` *code module*.


could also be called backports, and possibly _backports

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

src/stdio_mgr/compat.py

bskinn

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.

bskinn · 2019-09-11T13:49:01Z

src/stdio_mgr/compat.py

+    http://www.github.com/bskinn/stdio-mgr
+
+**Documentation**
+    See README.rst at the GitHub repository


These documentation links in all source files should point to the new RtD docs: https://stdio-mgr.readthedocs.io/

src/stdio_mgr/compat.py

bskinn · 2019-09-11T14:43:39Z

src/stdio_mgr/types.py

+        return self
+
+
+class FakeIOTuple(StdioTupleBase):


Eventually these will interact with ClosingStdioTuple somewhere?

How do you envision ClosingStdioTuple interacting with, e.g., _MultiCloseContextManager?

bskinn · 2019-09-11T14:46:29Z

src/stdio_mgr/types.py

+    """Tuple context manager of stdin, stdout and stderr-like objects."""
+
+
+class AnyIOTuple(StdioTupleBase):


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.

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.

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.

bskinn · 2019-09-11T14:50:10Z

src/stdio_mgr/types.py

+    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):


If the type check in TextIOTuple.__new__() were stronger, this could just be:

try: return TextIOTuple(items) except {{{RelevantError}}}: return FakeIOTuple(items)

bskinn · 2019-09-11T14:51:30Z

tests/test_types.py

+    assert str(err.value) == "underlying buffer has been detached"
+
+
+class _SafeCloseIOTuple(ClosingStdioTuple):


Why is this defined de novo here? Is it just temporary, until it becomes sensible to move it to the actual codebase?

bskinn · 2019-09-11T14:52:46Z

tests/test_types.py

+    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


Clarify/typo-check comment, pls

bskinn · 2019-09-11T22:56:36Z

Please go ahead and rebase this onto master, if you don't think attempting a subsequent rebase of reconf (#78) will foul it.

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.

bskinn · 2019-09-12T01:11:07Z

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.

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.

bskinn · 2019-09-12T19:40:31Z

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.

bskinn · 2019-09-13T16:14:58Z

@jayvdb Machinery is in place to auto-update bskinn/pr-foo branches, tracking the jayvdb/foo branches in PRs, so that the inheritance diagrams &c in the docs are up to date with the state of the PR. It's in a run-every-minute cronjob, so updates should be fast.

Do you want me to add you to the email notifications, when the pr-foo docs branches get pushed?

bskinn · 2019-09-20T02:59:08Z

@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?

jayvdb · 2019-09-20T03:08:28Z

Sorry - been a bit busy elsewhere. Also, do you know what is causing the CI problem atm. I couldnt quickly spot the problem.

bskinn · 2019-09-20T10:37:50Z

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

bskinn · 2019-09-20T11:01:27Z

Ah, that makes sense. The autodoc in the Sphinx docs AFAIK can only be pointed at individual modules, and right now only stdio_mgr.py is being documented. First thing to do is add new .. automodule:: directives to doc/source/api.rst for each of stdio_mgr.types and stdio_mgr.triple.

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.

jayvdb · 2019-09-20T11:10:19Z

Thanks. I'll try to get an update done over the weekend.

jayvdb requested a review from bskinn September 6, 2019 10:36

jayvdb mentioned this pull request Sep 6, 2019

POC: Reconfigure existing streams #78

Open

jayvdb commented Sep 6, 2019

View reviewed changes

jayvdb force-pushed the types branch 2 times, most recently from 20be80c to b2996d5 Compare September 7, 2019 04:53

bskinn requested changes Sep 8, 2019

View reviewed changes

jayvdb commented Sep 8, 2019

View reviewed changes

jayvdb force-pushed the types branch 2 times, most recently from 63397b5 to 9446d6e Compare September 9, 2019 16:39

jayvdb commented Sep 9, 2019

View reviewed changes

src/stdio_mgr/types.py Outdated Show resolved Hide resolved

jayvdb commented Sep 9, 2019

View reviewed changes

src/stdio_mgr/compat.py Show resolved Hide resolved

jayvdb changed the title ~~Base types~~ WIP: Base types Sep 10, 2019

bskinn requested changes Sep 11, 2019

View reviewed changes

jayvdb added 2 commits September 12, 2019 07:03

Backport contextlib.AbstractContextManager

8d01760

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.

TEST: Add MRO test to track changes

e81ad66

jayvdb force-pushed the types branch 3 times, most recently from 1f3f8fa to 376c194 Compare September 12, 2019 01:02

jayvdb force-pushed the types branch 2 times, most recently from 180cd54 to ee9edda Compare September 12, 2019 03:38

jayvdb force-pushed the types branch from ee9edda to 80f8b03 Compare September 12, 2019 03:40

bskinn mentioned this pull request Dec 16, 2019

Document prior art #88

Open

		"""Tuple context manager of stdin, stdout and stderr-like objects."""


		class AnyIOTuple(StdioTupleBase):

		assert str(err.value) == "underlying buffer has been detached"


		class _SafeCloseIOTuple(ClosingStdioTuple):

WIP: Base types #76

Are you sure you want to change the base?

WIP: Base types #76

Conversation

jayvdb commented Sep 6, 2019 • edited

codecov-io commented Sep 6, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskinn commented Sep 6, 2019

bskinn commented Sep 6, 2019 • edited

jayvdb commented Sep 6, 2019

bskinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bskinn commented Sep 11, 2019

bskinn commented Sep 12, 2019

bskinn commented Sep 12, 2019 • edited

bskinn commented Sep 13, 2019

bskinn commented Sep 20, 2019 • edited

jayvdb commented Sep 20, 2019

bskinn commented Sep 20, 2019

bskinn commented Sep 20, 2019

jayvdb commented Sep 20, 2019

jayvdb commented Sep 6, 2019 •

edited

codecov-io commented Sep 6, 2019 •

edited

bskinn commented Sep 6, 2019 •

edited

bskinn commented Sep 12, 2019 •

edited

bskinn commented Sep 20, 2019 •

edited