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

fix LoggerConfig inconsistency bug #233

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

mski-iksm
Copy link
Contributor

gokart.build() has a feature to disable low important logs.
However, the implementation has inconsistency,

  • logging.disable(self.level): disable any logs with level same or below the self.level
  • self.logger.setLevel(self.level): enable any logs same or above the self.level

This is because logging.Logger.setLevel and logging.disable has the following difference.

setLevel: Logging messages which are less severe than level will be ignored
logging.disable: disable all logging calls of severity level and below

This PR fix the inconsistency by disabling any logs with level below the self.level

@mski-iksm mski-iksm requested review from hirosassa, Hi-king and vaaaaanquish and removed request for hirosassa and Hi-king August 9, 2021 07:06
gokart/build.py Outdated
self.logger.setLevel(self.level)
return self

def __exit__(self, exception_type, exception_value, traceback):
logging.disable(self.default_level)
logging.disable(self.default_level - 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to be 10? If not, use sys.maxsize to make the meaning clearer.

Copy link
Contributor Author

@mski-iksm mski-iksm Aug 9, 2021

Choose a reason for hiding this comment

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

@vaaaaanquish
I think it should be 10, because we want to change "disabling logs with level self.default_level and bellow" to "disabling logs with levels bellow self.default_level"

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It seems like a good idea to put it in the comments.

Copy link
Contributor

@vaaaaanquish vaaaaanquish left a comment

Choose a reason for hiding this comment

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

LGTM 🆒

@hirosassa
Copy link
Collaborator

Thanks! LGTM

@hirosassa hirosassa merged commit 1654f00 into m3dev:master Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants