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

Add option to ignore empty context in monolog #233

Closed
wants to merge 1 commit into from

Conversation

flippedbits
Copy link

Added the option to allow ignoring of empty context when writing to monolog. I reason for this is to slim down the number of lines the logging produces and the double [] [] monolog is writing does not need to be there.

@PabloKowalczyk
Copy link
Collaborator

Could you add end-to-end test which will test added behavior?

@PabloKowalczyk PabloKowalczyk added this to the v2.1 milestone Jun 12, 2019
@flippedbits
Copy link
Author

flippedbits commented Jun 12, 2019

Looking closer at this, it doesn't look like any of the logging currently uses the context parameter anyways. Would it be simpler to just default the ignore empty context parameter to true?

@PabloKowalczyk
Copy link
Collaborator

It is tempting to make ignore empty context true by default, but it can be BC break for someone who parses logs, so it is better to leave it as false and change it to default true on next major version.
WDYT?

@flippedbits
Copy link
Author

Yeah I think that would be fine.

On a related note, what do you think of the idea of a simple custom log format config option. Then in the crunz.yml someone could do something like: log_custom_format: '%desc%(%cmd%)%nl%%output%

%desc% = description %cmd% = command %nl% = PHP_EOL %output% = script output

@PabloKowalczyk
Copy link
Collaborator

On a related note, what do you think of the idea of a simple custom log format config option. Then in the crunz.yml someone could do something like: log_custom_format: '%desc%(%cmd%)%nl%%output%

We can go for it for v2.1, would you like to provide PR?

@flippedbits
Copy link
Author

Yes I can provide a PR

@PabloKowalczyk
Copy link
Collaborator

@rrushton do you need some help with this PR?

@flippedbits
Copy link
Author

Apologies, work has been very busy and I have not had a chance to get back to this.

@PabloKowalczyk
Copy link
Collaborator

Merged manually in 75ba071, thanks @rrushton.

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.

3 participants