-
-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
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.
Looks great! Just one comment.
try: | ||
sys.exit(main()) | ||
except KeyboardInterrupt: | ||
logger.info("Received wish to exit by CTRL+C, exiting immediately.") |
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.
One nice QoL thing might be to just do a final print_timer()
-- what do you think? 😛
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.
Hm, you'd probably not see that unless you're running from source. In addition, the "intended" way to exit is to enable the "stop queueing" logic, and then to Ctrl+C or close the window, which would print it twice :/
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.
Fair enough -- I sometimes interrupt it mid-game and then surrender manually, so was thinking of that use case, but it's fairly minor 😛
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.
Hm, I think we can improve this catch to see from which logic step it was called, so maybe we can add it as a small improvement in another PR :)
Description
This PR adds changes that catch some new errors that may occur since we switched to HTTP requests.
We now also catch KeyboardInterrupt to correctly display that we know we should exit instead of printing a random stack trace.
In addition, a fix has been added to check if our session has expired.
Fixes #133
Fixes #134