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

Give logger a null handler on start up #2944

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

matthewrmshin
Copy link
Contributor

So, cylc logger will always have a handler. CylcOptionParser will ensure
that commands will have a valid logging handler. Scheduler will do
likewise for daemonized suites.

Use CylcOptionParser for all remaining user commands:

  • cylc cycle-point
  • cylc documentation
  • cylc get-gui-config
  • cylc get-site-config

This ensures that these commands will have logging handlers set.

Fix #2940.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label Feb 6, 2019
@matthewrmshin matthewrmshin added this to the next-release milestone Feb 6, 2019
@matthewrmshin matthewrmshin self-assigned this Feb 6, 2019
@matthewrmshin
Copy link
Contributor Author

matthewrmshin commented Feb 6, 2019

We should revisit logging as part of the 2to3 effort (or soon after) and have a better strategy.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@kinow
Copy link
Member

kinow commented Feb 6, 2019

The formatting of the help screen of some commands changed, but only really small changes, which are OK IMO. e.g.

image

Before the change, the cylc get-site-config had no difference when running with --verbose and --debug. Not it appears to be working OK 😬

image

So, cylc logger will always have a handler. CylcOptionParser will ensure
that commands will have a valid logging handler. Scheduler will do
likewise for daemonized suites.

Use CylcOptionParser for all remaining user commands:
* cylc cycle-point
* cylc documentation
* cylc get-gui-config
* cylc get-site-config

This ensures that these commands will have logging handlers set.
@matthewrmshin matthewrmshin force-pushed the fix-no-logging-handler branch from 0c47441 to 59caceb Compare February 7, 2019 10:55
@matthewrmshin
Copy link
Contributor Author

(Sorry, just re-based.) I have also added a compatibility check - as Python <2.7 does not have logging.NullHandler.

@kinow
Copy link
Member

kinow commented Feb 7, 2019

Not a problem, just one line change with the new if. Assuming Travis/Codacy are still happy, keeping my +1. Thanks

@oliver-sanders oliver-sanders merged commit 94f2732 into cylc:master Feb 7, 2019
@oliver-sanders
Copy link
Member

Sorry I didn't refresh this page before merging so missed your comments, feel free to revert.

@matthewrmshin matthewrmshin deleted the fix-no-logging-handler branch February 8, 2019 09:21
@matthewrmshin matthewrmshin mentioned this pull request Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants