fetch-configlet: use logging module instead of print#2188
Conversation
|
|
||
|
|
||
| logging.basicConfig(level=logging.INFO) | ||
| logger = logging.getLogger(__file__) |
There was a problem hiding this comment.
This should be __name__ not __file__.
yawpitch
left a comment
There was a problem hiding this comment.
Looks good except for the logging.getLogger(__file__) ... IIRC the logging module assumes that a dot in the name indicates a child logger, so the relative file path would make it go a bit nuts if we ever added submodules to this.
It won't get in the way as is, so I'll approve, but I'd still fix that one before merge if you can.
| logger.error(f"Received unexpected {status} response from API: {response}") | ||
| return 1 |
There was a problem hiding this comment.
FWIW, I originally did that as sys.exit() because my thinking was a client error other than a 403 from the API is probably a permanent condition within the lifetime of the script run. Probably doesn't matter either way.
There was a problem hiding this comment.
Intent noted. It still seems the wrong approach to allow the function itself to trigger a full program exit. If we wanted to keep that short-circuit functionality, the best approach would probably be to raise some sort of unique exception to allow the caller to handle the failure however it wishes.
No description provided.