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

Revert #43641 #44079

Closed

Conversation

ericphanson
Copy link
Contributor

@ericphanson ericphanson commented Feb 8, 2022

Reverts #43641 as discussed there and in #44016

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2022

As you mentioned on that PR though, it seem we do expect it to support minlevel and other standard logger features such as maxlog

@ericphanson
Copy link
Contributor Author

Hm ok :). Do we want to keep #43641 then? Do we want this additionally? I do think it's useful on it's own. Maybe I should make these two separate PRs so they can be discussed individually.

@ericphanson ericphanson force-pushed the eph/revert-testlogger-maxlog branch from ebb731a to d9f4f18 Compare February 8, 2022 19:46
@ericphanson ericphanson changed the title Document and export TestLogger and LogRecord; revert #43641 Revert #43641 Feb 8, 2022
@ericphanson
Copy link
Contributor Author

ericphanson commented Feb 8, 2022

Ok, I made this PR just the revert and pulled the other commit out to #44080.

And then I guess maybe we should close this if we want a TestLogger to act kind of like a usual logger, supporting maxlog and minlevel? cc @fredrikekre

@fredrikekre
Copy link
Member

And then I guess maybe we should close this if we want a TestLogger to act kind of like a usual logger, supporting maxlog and minlevel?

Lets make respecting max log optional, just like the min level then? By default I think it should capture everything, but if you want to test something else you can configure it.

@ericphanson
Copy link
Contributor Author

Lets make respecting max log optional, just like the min level then?

Sure, do you think just respect_maxlog=true in the constructor and store the bool in the struct?

@fredrikekre
Copy link
Member

Yea, something like that.

@ericphanson
Copy link
Contributor Author

Closing in favor of #44085

@ericphanson ericphanson closed this Feb 9, 2022
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