Skip to content

Conversation

@acoulton
Copy link
Member

@acoulton acoulton commented Oct 8, 2020

This avoids the risk of errors setting the cookie if the process has
already sent output to the console.

This avoids the risk of errors setting the cookie if the process has
already sent output to the console.
@acoulton acoulton requested a review from craig410 October 8, 2020 22:05
@acoulton
Copy link
Member Author

acoulton commented Oct 8, 2020

@craig410 this isn't quite as clean as I'd like, obviously want it to detect the CLI automatically but that state needs to be injected into the class otherwise it prevents all the unit tests running the real code.

Which means I can only really pass it into the init method - if a logger calls ::get() before it initialises (e.g. due to an error early in bootstrap) it will get -unset- instead of the CLI value. But I think that's a tolerable edge case for keeping it a small change and testable?

Copy link
Member

@craig410 craig410 left a comment

Choose a reason for hiding this comment

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

Yeah I agree, theres not any other way to do it and overall it's neater than having to wrap every usage of it per application

@acoulton acoulton merged commit 4aa2f05 into 1.x Oct 9, 2020
@acoulton acoulton deleted the 1.5-logging-cli-device-id branch October 9, 2020 14:01
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