Use base TextIOWrapper for stdout and stderr

[jayvdb]

Follow on from #25 solving #24

[codecov-io]

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #26   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          52     76   +24     
=====================================
+ Hits           52     76   +24

Impacted Files	Coverage Δ
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 0d434b6...ebe8e3d. Read the comment docs.

[jayvdb]

With this I was able to get the panflute stdio tested. https://travis-ci.org/jayvdb/panflute/builds/564574216

I've also included the encoded bytestream stdin patch, and tried to ensure that all handles are closed even if there is an exception while leaving the context manager. Let me know if there is a better way to write that - it is a bit messy. I think I can write a with to catch exceptions while closing the handles, and then ignore only ValueError - that should be a bit neater. I'll add tests for them once I've done more testing.

Whereas with stdin I tried to stick very close to the real stdin, here I havent tried as hard, as these use a random access buffer. IMO this looser implementation is sufficient for 95% of users - the other 5% probably need a specifically crafted solution anyway, and wont use a helper like this package, or they'll come here and provide a better solution.

[jayvdb]

Raw binary stdio removed from this PR, and the rest cleaned up. Looking forward to fixing any of my laziness that your keen eyes spot.

[bskinn]

flake8 is now in CI -- please rebase/merge atop master and fix any complaints that emerge. I'll continue reviewing at that point.

[jayvdb]

This was rebased.

[bskinn]

<nod>, sorry, didn't have a chance to get to it today. Hopefully no later than end-of-day Friday US Eastern.

[jayvdb]

I didnt get time to think about encodings. I think this is mergable without customisable encodings, and #33 provides a way to change the encodings without exposing lots of params in the constructor.

[bskinn]

Per the side-streamed comments, after a few minor documentation/typo fixes, and perhaps some decisions on the public API, I think I'm good to merge this.

Should note, I ran a local test of your output-handles branch in my project that uses stdio-mgr most heavily, and it worked smoothly, on both Debian and Windows. So, 👍.

Can't say it enough... again, thank you very much for putting so much work into these improvements.

[jayvdb]

I think I got all the docs fixed, but there are definitely bits where the internals could be explained better.

I prefer to keep RandomTextIO public with Teestdin already public. Its constructor takes no args at the moment, and I believe that any future args will have defaults, so there shouldnt be breaking changes in the immediate future. In the event it becomes useful to other people, but needs to change, we could always use a new class for the internals and deprecate the old one until it can be removed in a major release.

[bskinn]

I think I got all the docs fixed, but there are definitely bits where the internals could be explained better.

Would you rather buff the docs as part of this PR, or get this merged and work on them subsequently?

I prefer to keep RandomTextIO public with Teestdin already public. Its constructor takes no args at the moment, and I believe that any future args will have defaults, so there shouldnt be breaking changes in the immediate future. In the event it becomes useful to other people, but needs to change, we could always use a new class for the internals and deprecate the old one until it can be removed in a major release.

Good point. This probably does argue in favor of just leaving them public. One advantage of the stub superclass I mentioned in the in situ comment, though, is that then users wouldn't have to care as much which specific class they should be isinstance-ing for on a given stdxx.

[jayvdb]

I'll do another push in a few hours based on discussion.

[bskinn]

Another doc change

[bskinn]

Should the stream created from init_text in TeeStdin also be a _PersistedBytesIO?

And then, replicate the logic of the getvalue in RandomTextIO? (Hm, that's not DRY -- any way to sanely push getvalue into a superclass?)

[jayvdb]

Should the stream created from init_text in TeeStdin also be a _PersistedBytesIO?

Still getting my head around this. First step was working out if it was a regression. If i.getvalue() is not going to work after i.close(), then the .getvalue() needs to be fixed: #39

I am not convinced of the need to have the i.getvalue() work after .close(), or that it cant be derived from the init_text and i.append(..) and o.getvalue(). The inputs are known, and the output is able to be deduced from o.getvalue(). If anything, I believe the only state which isnt known is the seek position on the input buffer, and that position could be stored without storing the entire buffer. But that doesnt cater for when people write to i.buffer directly, and I think we need use cases and test cases around that before committing to any behaviour. Still thinking about those cases. Only init_text and i.append(..) can write to ._buf, as i.buffer is a BufferedReader. So I think i.getvalue() after i.close() isnt so useful except for debugging, and someone debugging can usually use o.getvalue() to deduce how much of the input was consumed. Adding debug level logging would be more beneficial.

[bskinn]

I am not convinced of the need to have the i.getvalue() work after .close(), or that it cant be derived from the init_text and i.append(..) and o.getvalue(). The inputs are known, and the output is able to be deduced from o.getvalue().

Agreed. With testing being the main target application, the tester should have sufficient control over what's fed into i to not need i.getvalue after close. There may be some other applications that arise where the inputs will not be known ahead of time, but for now I think you're right, it's YAGNI.

[bskinn]

I think this PR is ready to merge, after a final rebase onto master following the merge of #39 (good catch, btw).

That will let us start with a clean issue/PR for evaluating docs/docstrings, and the sufficiency of the test suite as it sits. I plan to run mutmut over the test suite as part of this. Getting this PR merged will then provide a platform for addressing the other issues flagged for v2.0, too.

We'll also want to decide on the public API. Probably best to run that discussion in a new issue: #42 created.

[jayvdb]

I think I've got most of the changes requested, and added some more tests and improved the names/docs of them, but not covering items which have been deferred to other issues.

Happy to do more fixups if you see something nasty. Otherwise I'll do any fixups after merge, and work on bytes/encoding.

[bskinn]

Nope, I'm good to merge this as-is, and let further work move along with this as the baseline.

There may be some stuff to find and fix, but that can happen as it comes up.