Skip to content

add types to logging #247

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

Merged
merged 1 commit into from
Jun 5, 2016
Merged

add types to logging #247

merged 1 commit into from
Jun 5, 2016

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented Jun 3, 2016

Add types to the logging module, in a python 2-3 compatible version.

@gvanrossum gvanrossum merged commit a581397 into python:master Jun 5, 2016
@gvanrossum
Copy link
Member

That's a lot of code... I will have to check whether it really works. If I find a flaw I can't quickly fix I may have to roll back. But for now, benefit of the doubt. Thanks!

@gvanrossum
Copy link
Member

I found a few things.

  • instead of TextIO, it should use IO[str](since sys.stdout is the latter in 2.7)
  • lots of code uses logging.WARN instead of logging.WARNING. I fixed these in d75e2ab

A few more I'm not sure whether to fix:

  • the message argument to error() etc. can be unicode in Python 2
  • the message argument to error() etc. can be a non-string (apparently str() is applied somewhere)
  • there are undocumented public-seeming attributes that are commonly used, e.g. Logger.level

I think I'll fix the unicode one (it's easy to use typing.Text instead) but I'm not sure about the others. It's true these are undocumented but if they are actual behavior that lots of code relies on I think we may have to add them.

@gvanrossum
Copy link
Member

gvanrossum commented Jun 6, 2016

Also, log() takes an initial argument lvl: int -- I'll add this while I'm at it.

Done: 8ee3123

@tharvik
Copy link
Contributor Author

tharvik commented Jun 6, 2016

  • lots of code uses logging.WARN instead of logging.WARNING.
  • there are undocumented public-seeming attributes that are commonly used, e.g. Logger.level

Theses codes are wrong, sadly, but for now, I'll add them. Do you have a list of wanted attributes?

  • the message argument to error() etc. can be unicode in Python 2

Let's switch to Text then.

  • the message argument to error() etc. can be a non-string (apparently str() is applied somewhere)

And from Text, here goes Any. But it is still wrong code (which, in a way, is supposed to be found by mypy).

@gvanrossum
Copy link
Member

I fixed the one place I found that used logger.level in our code; I think it's pretty clear they were just taking a shortcut.

Regarding the message argument, I think it should be Text everywhere (and I've already made it that -- it should work because Text is defined in typing.pyi in typeshed, even though it's not yet in most runtimes since we're waiting for 3.5.2 to push out a new version).

@tharvik tharvik deleted the add_logging branch June 6, 2016 15:48
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.

2 participants