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

Check for isatty() is incorrect. #2

Open
GrahamDumpleton opened this issue May 13, 2014 · 0 comments
Open

Check for isatty() is incorrect. #2

GrahamDumpleton opened this issue May 13, 2014 · 0 comments

Comments

@GrahamDumpleton
Copy link

How you are checking for whether a file object is a tty is technically wrong.

File like objects provide a isatty() method themselves. You should not be accessing any associated fileno attribute and then using os.isatty().

If you are going to honour properly whether the file object is a tty, instead of using:

    try:
        fn = std.fileno()
    except:
        fn = None

    if fn is None or os.isatty(fn):
        handler.setFormatter(
            ColorFormatter(
                '$COLOR%(asctime)s $BOLD$COLOR%(name)s'
                ' %(funcName)s:%(lineno)d $RESET %(message)s'))

you should be using:

    isatty = False

    try:
        # The isatty() method is actually optional. Assume False
        # if it doesn't exist.
        isatty = std.isatty()
    except AttributeError:
        pass

    if isatty:
        handler.setFormatter(
            ColorFormatter(
                '$COLOR%(asctime)s $BOLD$COLOR%(name)s'
                ' %(funcName)s:%(lineno)d $RESET %(message)s'))

That you assume that you can still add the formatter when the fileno attribute doesn't exist is a bit worrying as it would be quite typical that if a file like object explicitly has a isatty() method which returns False that it will not have the fileno attribute.

So what it seems is that you are probably trying to say, that if the file like object wraps a file descriptor and that file descriptor is not a tty but a normal file, then don't add the formatter. For anything else you then assume that adding a formatter is fine, even though the file like object is explicitly saying it is not a tty.

You may have thought this was a good approach as it allows someone to supply an instance of StringIO and formatting will still be added, but it breaks down in other cases.

As an example, in Apache/mod_wsgi the stdout and stderr streams are replaced with a file like object which routes anything into the Apache logging system. The isatty() method is correctly provided and returns False and there is no fileno attribute as there is no file descriptor as anything gets pushed out via an API call instead and not to a file.

So the way you have done it results in all sorts of formatting appearing in the Apache error logs when it shouldn't be.

I suspect you probably now have stuff being dependent on this arguably incorrect behaviour so it is going to be hard to change, but it is arguably going about it the wrong way and there should have been some other way to override if formatting was added when directed output to something that wasn't actually a tty, rather than making the assumption you have.

BTW, naked except clauses are generally bad practice as it could catch and ignore exceptions which don't derive from Exception which shouldn't be ignored, such as SystemExit exception generated by sys.exit().

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

No branches or pull requests

1 participant