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

Bring back #disable! support #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bring back #disable! support #15

wants to merge 1 commit into from

Conversation

stevenharman
Copy link
Contributor

@stevenharman stevenharman commented Jul 26, 2021

As part of #7 we lost the ability to totally disable the logger, across all threads. It's unclear if this was intentional, but I am guessing it was not. This will restore the behavior by returning a new NullOutput instance when trying to write while disabled. This might cause A LOT of object allocations, all with short lives. Hopefully. If so, we could consider some form of memoization of the instance.

Alternatively, we might choose that we don't actually want this feature. In which case we need to update the README and probably mention it in the CHANGELOG.

NOTE: This PR also introduces a #enable!, which would close #4.

Thoughts?

As part of [#7](#7) we lost the
ability to totally disable the logger, across all threads. It's unclear
if this was intentional, but I am guessing it was not. This will restore
the behavior by returning a new `NullOutput` instance when trying to
write while disabled.

This might cause A LOT of object allocations, all with short lives.
Hopefully. If so, we could consider some form of memoization of the
instance.
@mathias
Copy link

mathias commented Sep 30, 2021

I'd like to get this merged. Turns out addons.h.c depends on disable!

@stevenharman
Copy link
Contributor Author

stevenharman commented Oct 4, 2021

I'm not 💯 sure this implementation works as intended (i.e., can you re-enable in the block form if disable! has been called? Do we even want to allow that?) and have a small concern about the performance implications of allocating a new NullOutput instance on each write. Thoughts?

If you need disable!, you might instead be able to silence each test via an RSpec around block?

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.

enable! method needed for conditionally enabling l2meter output in tests
2 participants