-
Notifications
You must be signed in to change notification settings - Fork 972
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
default config added to logger; logs added for job clearing/cancelling #193
Conversation
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zcking, thanks for the additions! Overall it looks really good. I've merged master into this branch and did some minor adjustments:
- Changed the level to debug to be in line with other log message (see Change logging level to debug #361).
- The
ValueError
exception only occurs if the job is not scheduled so changed the message to reflect that.
The only thing I'm not sure about is the basicConfig
, let's talk about that a bit more.
schedule/__init__.py
Outdated
logging.basicConfig( | ||
format='%(asctime)-15s - %(levelname)s (%(name)s) - %(message)s' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.basicConfig( | |
format='%(asctime)-15s - %(levelname)s (%(name)s) - %(message)s' | |
) |
logging.basicConfig
configures the root logger. Subsequent calls to basicConfig are ignored.
With this addition, if the schedule library is imported at the top of the file, the root logger is configured using the schedule basicConfig
. Now, if the the program itself calls basicConfig
to configure the root logger, that config will be ignored because it has already been configured by schedule. This is an example of such:
import logging
import schedule
logging.basicConfig(format='CUSTOMFORMAT: %(message)s', level=logging.DEBUG)
logging.info("test")
I would expect this code to print test
, but with the proposed change it doesn't print anything.
This will be quite confusing for users. Therefore I'm proposing to remove this basicConfig
call and merge the pr without it. Let me know what you think!
Merged master into this branch, updated the docs and removed the default logger configuration. Ready to merge 🚀 |
I added a default configuration for the logger, as recommended by the logging docs. I then added a logging statement to
cancel_job()
andclear()
as well as fixed a typo in one of the existing log statements.In
cancel_job()
instead of silently passing on the exception, this will log the exception and continue.The .gitignore change is just for folks like me who use Idea IDEs such as PyCharm which creates a .idea/ folder with .iml files for some meta info the IDE uses.
Here is an example interactive session demonstrating the new logging: