search

danijar / handout

Turn Python scripts into handouts with Markdown and figures
Apache License 2.0
2.01k stars 106 forks source link

logger creation removed from __init__.py #27

Closed epogrebnyak closed 5 years ago

epogrebnyak commented 5 years ago

Fixes #11

danijar commented 5 years ago

As mentioned in #11, I think the logger should be created on import so that the user can do logging.getLogger('handout') to change the log level etc. What do you think?

epogrebnyak commented 5 years ago

Here is an idea - one function is logger_config(), and it is called on import, the other fucniton in logger_provide() and it simply returns a proper logger in Handout.

This way the logger is available upon import and configurable by the user, but we keep __init__.py cleaner, more avoiding the hidden state.

danijar commented 5 years ago

This seems a bit too complicated to me. I don't think moving the logic from __init__.py into tools.py makes it less hidden. On the contrary, now it's not clear anymore where and how often the logger is created. Moreover, I don't see the need for the get_logger() function that is just an alias for an existing function in the logging module. In other words, let's just add the one line to __init__.py to check whether there are any handlers already, if you don't mind.

epogrebnyak commented 5 years ago

Well, you can make a one-liner change in __init__.py, but the place is rather hidden, otherwise it would not have taken that much time to find the source of error for #11.

Something happening in __init__.py except for the imports in a bit shaky.

get_logger is not just a sugar for the logging function - it returns a specific logger named 'handout'. Otherwise it is a question how a programmer knows to use this name for the logger, also for configuring it to a different level?

moving the logic from init.py into tools.py makes it less hidden.

It does - there is a dedicated filelogger.py now. If I saw one in codebase, would have saved me a ton of time tracing the duplicate messages error, just as example.

epogrebnyak commented 5 years ago

I can see some preference in keeping the codebase compact without logger.py though, so the decision is up to you.

danijar commented 5 years ago

You raise a valid point. I added the one line check and put a comment in the Handout class to tell contributors where to look for the logging configuration. As you said in your second comment, having a separate file for the logger seems a bit too much to me. There are many parts of the code that could all have separate files but every time we do that the codes gets more spread out and harder to edit. I'm closing the PR since I've added the fix -- thanks a lot for finding the bug in the first place!

epogrebnyak commented 5 years ago

Dev comment is a good idea!