Logging instead of print#860
Conversation
Reason for the change of the format string is not to change the output on the command line. A nice formater may be desirable in the long run.
These log messages are not that important to the average user. Since the default log level has been set to "info", the log level of these messages has to be adapted in order to hide them from the user.
jgosmann
left a comment
jgosmann
left a comment
There was a problem hiding this comment.
I'm not sure whether we enforce this for Nengo GUI, but it is good practice to keep the first line of the commit message to 50 characters.
| print("Resuming...") | ||
| logger.info("Resuming...") | ||
| else: | ||
| print("No confirmation received. Resuming...") |
There was a problem hiding this comment.
These last two print statements should either stay prints or should be converted to sys.stdout.write because this output is part of direct user interaction and should never be redirected to a log file for example.
There was a problem hiding this comment.
Actually, I might argue that the other prints in this function should stay prints too. Though it is not as clear cut for those.
| for thread, _ in self.server.requests) | ||
| if n_zombie > 0: | ||
| print("%d zombie threads will close abruptly" % n_zombie) | ||
| logger.warning("%d zombie threads will close abruptly", |
There was a problem hiding this comment.
I'd probably make this a debug message. It isn't really relevant to the user.
|
|
||
| def old_main(): | ||
| print("'nengo_gui' has been renamed to 'nengo'.") | ||
| print("Please run 'nengo' in the future to avoid this message!\n") |
There was a problem hiding this comment.
This should always be visible to the user and not end up in some log file. Thus, it should stay a print or even better go to sys.stderr.
| p1 = getpass("Enter password: ") | ||
| if p0 == p1: | ||
| return p0 | ||
| print("Passwords do not match. Please try again.") |
There was a problem hiding this comment.
This is also code with direct user interaction via stdin and stdout, thus it should stay a print.
|
|
||
| def log_message(self, format, *args): | ||
| logger.info(format, *args) | ||
| logger.debug(format, *args) |
There was a problem hiding this comment.
What are “these” log messages? It seems that this function isn't called in normal operation. Maybe in some specific error cases? But if it is error cases a higher log level might be better?
There was a problem hiding this comment.
This function is called from the web server base class and is called once for each incoming request.
There was a problem hiding this comment.
You're right, for some reason those messages did not appear on my Macbook (potentially I was running the wrong version of Nengo GUI). I agree that the debug log level makes more sense.
Keep prints where they are in order.
|
Added a commit to change things back to prints where appropriate. LGTM now. 🍰 |
It seems safe to me to replace this `warns` call with a logging call and it makes things consistent with the other logging call for error output. It seems to also give a nicer output in the notebook.
2 participants
This pull request fixes #290. All modules fetch a module-specific
loggerobject which is then used to log messages previously printed withprint, orlogging.without aloggerobject.This patchset should not significantly change the output of nengo_gui visible to the user. A future improvement would be a nice formater, which may print the log severity and other information. Note that all messages are now written to
stderrinstead ofstdout.Note that this patch set touches various places all over the code, it should thus be reviewed thoroughly.
Things which need to be checked before merging this pull request:
warnings-module inipython.pybe safely replaced by calls tologging? My guess was no, so I didn't change anything there.