Skip to content

Conversation

@erickhun
Copy link
Collaborator

This PR will :

  • allow to change verbosity by setting the LOG_LEVEL in the env variable
  • "silent" all logs lower than the NOTICE level

I use Monolog levels for now until we decide the final ones

@colinscape @esclapes Can I have your eyes on this ?

Copy link

@esclapes esclapes left a comment

Choose a reason for hiding this comment

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

Hey @erickhun, thanks for this work 🌮

I think monolog already supports this feature. If I am not mistaken we could configure it by passing it as the second parameter to the StreamHandler constructor:

$logLevelFromEnv = getenv(self::$logLevelEnvVar);
$handler = new StreamHandler('php://stdout', $logLevelFromEnv);

I may be missing some context on the requirements as there may be good reasons to build our own. If that's the case, this implementation LGTM 👍 I left a couple of small comments below. Nothing blocking!

Comment on lines 77 to 78
$logLevelFromEnv = getenv(self::$logLevelEnvVar);
$monologLevels = self::getLogger()->getLevels();

Choose a reason for hiding this comment

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

We could slide this two lines out and pass $logLevel to the setVerbosityLevel as an argument. That way this method becomes a setter that could be used to configure the verbosity level in user land. I don't think that's going to happen very often, though.

Comment on lines 92 to 93
// Optimization: we could move this at the singleton initialization
self::setVerbosityLevel();

Choose a reason for hiding this comment

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

:plus-one: to that optimization comment 👍

@erickhun
Copy link
Collaborator Author

@esclapes oh wow , great spot, I didn't spot that feature 🙈 This will greatly simplify the code 🤩. Let me rework the code and loop you again with this

@erickhun
Copy link
Collaborator Author

@esclapes I've reworked the code with using the StreamHandler feature for verbosity . Definitely cleaner (and better!) 🌮 🌮

@erickhun erickhun requested a review from esclapes December 12, 2019 07:53
Copy link

@colinscape colinscape 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! 👍

Copy link

@esclapes esclapes left a comment

Choose a reason for hiding this comment

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

Great that it helped @erickhun LGTM! 🌮 👍

@erickhun erickhun merged commit 01dcc01 into master Dec 13, 2019
@erickhun erickhun deleted the task/manage-verbosity branch December 13, 2019 02:27
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.

4 participants