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

Added interested levels support #8

Merged
merged 9 commits into from
Dec 18, 2017
Merged

Added interested levels support #8

merged 9 commits into from
Dec 18, 2017

Conversation

ilyaplot
Copy link
Contributor

Earlier definition of the list of levels of logging for target in any way did not influence its work.

@samdark
Copy link
Owner

samdark commented Dec 13, 2017

It was passing messages to PSR logger as is.

@ilyaplot
Copy link
Contributor Author

It was passing messages to PSR logger as is.

The list of levels can be specified in the config, but this does not lead to anything. I thought that it's better to implement the functionality than to mislead the developer. I tried to use SlackHandler only for certain levels, but I could not. Now this problem is solved.

@ilyaplot
Copy link
Contributor Author

You can, of course, define levels in the Rsr handler, but if I want to log only the TRACE level, then I get PROFILE levels too.

@samdark
Copy link
Owner

samdark commented Dec 14, 2017

Yes. It makes sense to add it. Please fix code (see travis build fail).

/**
* @var array
*/
private $_interestedLevels = null;
Copy link
Owner

Choose a reason for hiding this comment

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

No need to specify = null.

Copy link
Owner

Choose a reason for hiding this comment

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

Could be named _levels similar to base Target class.

public function init()
{
parent::init();
if (is_null($this->_interestedLevels)) {
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this check. At init() state it will be null all the time.

/**
* @var array
*/
private $_interestedLevels = null;
Copy link
Owner

Choose a reason for hiding this comment

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

Could be named _levels similar to base Target class.

{
parent::init();
if (is_null($this->_interestedLevels)) {
$this->_interestedLevels = $this->_psrLevels;
Copy link
Owner

Choose a reason for hiding this comment

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

I think that by default there should be both PSR and non-PSR levels. Would simplify code significantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not quite understand the point. You mean the division into PSR and non-PSR levels?

Copy link
Owner

@samdark samdark Dec 14, 2017

Choose a reason for hiding this comment

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

I mean $this->_interestedLevels = [all PSR and Yii levels here]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, _psrLevels will duplicate _levels. I left only _levels.

*/
public function setLevels($levels)
{
static $levelMap = [
Copy link
Owner

Choose a reason for hiding this comment

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

That would disallow using PSR levels in setLevels(). Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? _levels contains the PSR levels. I need to write tests for this.

@samdark samdark added this to the 1.0.5 milestone Dec 18, 2017
@samdark samdark merged commit c4a83c7 into samdark:master Dec 18, 2017
@samdark
Copy link
Owner

samdark commented Dec 18, 2017

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants