Skip to content
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

Improve UI when `pelican --listen` web server is quit #2684

Merged
merged 2 commits into from Jan 20, 2020
Merged

Conversation

@justinmayer
Copy link
Member

justinmayer commented Jan 17, 2020

Fixes #2676. Any comments from @getpelican/reviewers?

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Jan 17, 2020

I don't think logger.info is visible without -v or -D. Maybe logger.warning instead?

except KeyboardInterrupt:
stop_msg = "Keyboard interrupt received. Shutting down server."
logger.info(stop_msg)
print("\n{}".format(stop_msg))

This comment has been minimized.

Copy link
@iKevinY

iKevinY Jan 17, 2020

Member

Shouldn't we only have one of either logger.foo or print, otherwise it'll double-output the message (assuming the logging visibility is high enough)?

@justinmayer

This comment has been minimized.

Copy link
Member Author

justinmayer commented Jan 18, 2020

@avaris / @iKevinY: Thanks for the input. I tried replacing the logger.info() + print() combination with logger.warning(), but that yields a rather ugly and inaccurate WARNING prefix:

WARNING: Serving site at: 127.0.0.1:8000 - Tap CTRL-C to stop
^CWARNING:
  | Keyboard interrupt received. Shutting down server.

Whereas before it looked much more user-friendly:

Serving site at: 127.0.0.1:8000 - Tap CTRL-C to stop
^C
Keyboard interrupt received. Shutting down server.

Any suggestions as to how to reconcile this situation?

@iKevinY

This comment has been minimized.

Copy link
Member

iKevinY commented Jan 18, 2020

Hmm, agreed that since this is user-facing it would be nicer to have a print somewhere at least (and that the WARNING log is a bit unsightly). If the logger output isn't strictly necessary, could we get by with only a print here?

justinmayer added 2 commits Jan 17, 2020
This knowledge was heretofore assumed but is better made explicit.
Users were previously met with an ugly traceback. Now `pelican --listen`
invocations, when quit via CTRL-C, are followed instead by a more
user-friendly message.
@justinmayer justinmayer force-pushed the listen-interrupt branch from c23812c to fa71931 Jan 19, 2020
@justinmayer

This comment has been minimized.

Copy link
Member Author

justinmayer commented Jan 19, 2020

I modified the commits here to only print to the console, which seems like a workable approach to me. Any further comments on this PR?

Copy link
Member

iKevinY left a comment

LGTM

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Jan 19, 2020

Yes, warning will do that. I considered that as a necessary evil. Alternative would be dropping level to logging.INFO when --listen is provided but that would result in a really verbose output when combined with --autoreload.

I suppose print is the simpler solution here.

@avaris
avaris approved these changes Jan 19, 2020
@justinmayer

This comment has been minimized.

Copy link
Member Author

justinmayer commented Jan 20, 2020

Thanks for the input, @avaris and @iKevinY. 💫

@justinmayer justinmayer merged commit cf96c11 into master Jan 20, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@justinmayer justinmayer deleted the listen-interrupt branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.