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

Change default enabled behavior of Log class #115

Merged
merged 2 commits into from
Dec 5, 2019
Merged

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented Dec 5, 2019

  • If log is disabled, all levels of logging are expected to be blocked. Currently it doesn't block L0 including warnings, errors and level=0 logs. The enabled flag is practically meaningless (can just be replaced with log.level = 0).
  • It's reasonable to expect that new Log().log('hello')() would actually log something to the console. Currently log instances are disabled by default.
  • It's cumbersome for the users to have to do log.enable(true).setLevel(1) just to see some logs.

In both deck.gl and luma.gl, the log objects are enabled on initialization, e.g. https://github.com/uber/deck.gl/blob/7.3-release/modules/core/src/utils/log.js#L3
Therefore changing the default here does not affect the default behavior in our primary use cases.

@coveralls
Copy link

coveralls commented Dec 5, 2019

Coverage Status

Coverage increased (+4.3%) to 45.593% when pulling e76b55b on x/default-level into 4bd5096 on master.

@Pessimistress Pessimistress changed the base branch from x/log-level to master December 5, 2019 20:28
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

LGTM.

Maybe something can be done to document the changes?

@Pessimistress Pessimistress merged commit 1bc4627 into master Dec 5, 2019
@Pessimistress Pessimistress deleted the x/default-level branch December 5, 2019 21:00
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.

None yet

3 participants