Skip to content
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

utils.io.Term.cin/out/err -> utils.io.stdin/out/err #397

Merged
merged 2 commits into from Apr 27, 2011

Conversation

minrk
Copy link
Member

@minrk minrk commented Apr 20, 2011

There are a variety of methods that depend on io.Term existing, and some of these can be called before io.Term is created, and none of them check for its existence.

Possible solutions are:

  1. ensure io.Term always exists, but may be overridden
  2. check for existence of io.Term before using it

This PR implements 1., by adding simple Term=IOTerm() in utils.io.py.

An example of the failure: missing readline. init_readline() can display warnings via utils.warn, which depends on io.Term. However, init_io() must be called after init_readline(), so these warnings will always crash IPython.

@ellisonbg
Copy link
Member

I don't like this, but it is probably OK until we figure out a better way of initializing stuff like this. At some level, things like this and logging need to be setup at the Application level - IE, before things like InteractiveShell are created.

@minrk
Copy link
Member Author

minrk commented Apr 20, 2011

What is the argument against io.Term having default behavior of sys.stdin/out/err? In what cases would it be better for calls to utils.warn prior to io.Term configuration to raise an error of some kind (AttributeError at the moment) rather than just printing to stderr?

We need to ensure one of the following:

  1. the object is always valid
  2. the object is valid in the scopes from which it can be called

And we do neither, but code assumes we have done 2.

You are right that it should be configured at the application level like logging, but it should also be like logging and not raise an error if used prior to configuration. logging is silent if unconfigured, and I think sys.stdin/out/err is the most reasonable default for our object that's an abstraction of exactly those streams.

@ellisonbg
Copy link
Member

I think that stdout/stdin/stderr are fine defaults, I am just not excited about hardcoding defaults into a top-level module variable. Sort of goes against our model of defaults and configuration. I guess it would make sense to have a model that is similar to that of logging though, maybe have a get_term function in io.py. I do agree that it should work even before configuration like logging does. The most important aspect of Term is that other modules that need to use Term should get it in a way that is robust. The old, from io import Term is horrible because if io.Term changes, the code doesn't see that. That is why we removed io.Term in the first place. So maybe the logging model does make sense.

@minrk
Copy link
Member Author

minrk commented Apr 20, 2011

What about having top-level stdin/out/err like sys? (or cin/out/err, if that's preferable for some reason). Then it's quite familiar that the top-level object (io) is what you want to hold onto, and code can redirect io.stdout/err as it sees fit, just like it can with sys.stdout/err, and code that does one out=io.stdout will not follow redirection, just like you expect if you do out=sys.stdout.

io.Term looks like it might behave that way, but doesn't because we create a new Term object on reconfiguration. Note that we don't have to do this - there's no reason not to assign directly to io.Term.cout instead of creating a new Term object, and if we did that, then from utils.io import Term would still work.

Side question: Is there a reason that io.Term is capitalized?

@ellisonbg
Copy link
Member

I like the idea of echoing the design of sys.stdout and friends with io.stdout|stdin|stderr and I think this is probably the way to go. The naming of "Term" is historical and even the name term (which suggests "terminal" is horrible at this point. I don't see any reason to not move to this new API.

@minrk
Copy link
Member Author

minrk commented Apr 21, 2011

Okay, this has now been updated, such that io.Term.cin/out/err are no longer used, in favor of io.stdin/out/err. The behavior is now the same as sys.stdin/out/err, only IPython-wide, instead of sys-wide. The objects are also initialized to IOStream(stdin/out/err), so they are always defined.

The IOTerm class remains, for any cases where one wants to contain a triplet of stdin/out/err redirects in a single object, but it's used nowhere in the code.

@ellisonbg
Copy link
Member

This is wonderful. The changes are pretty simple. Have you tested the qt console with this branch as we point things to zmq enabled out streams? I don't see any issues though. Some things we should look at after this is merged:

  • Once the new config stuff is merged, we should get rid of IPython's custom warning stuff and just use the logging module.

@ellisonbg
Copy link
Member

Min, have you tested this on Windows? If not, could you do that (I don't have a Windows VM setup right now). Fernando wanted to make sure that the terminal still works with colors (try 1/0 to make sure colored exceptions are working) before merging. I need to get a VM setup for this stuff...

@minrk
Copy link
Member Author

minrk commented Apr 27, 2011

I didn't change how anything works on the inside of the IOStream class, so it should behave the same. I did test on Windows, and 1/0 does produce the expected colorful output. As there is no color related code in utils.io, I presume it is the linking of stdout/err to readline._outputfile that allows colors on Windows? That code is the same as before.

I'll just rebase on master, and it should be ready then.

Behavior is now the same as sys.stdin/out/err, and
defaults to those streams.
This allows io.stdout/err to be captured during tests
@minrk
Copy link
Member Author

minrk commented Apr 27, 2011

rebased on master, tests passing, colors working.

@ellisonbg
Copy link
Member

Great, I will merge shortly

On Wed, Apr 27, 2011 at 11:41 AM, minrk
reply@reply.github.com
wrote:

rebased on master, tests passing, colors working.

Reply to this email directly or view it on GitHub:
#397 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

ellisonbg added a commit that referenced this pull request Apr 27, 2011
utils.io.Term.cin/out/err -> utils.io.stdin/out/err
@ellisonbg ellisonbg merged commit a34642c into ipython:master Apr 27, 2011
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
utils.io.Term.cin/out/err -> utils.io.stdin/out/err
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants