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

Switch verbosity macros to use group argument #73

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Oct 21, 2022

As noted in this comment, there's currently a semantic error with the verbosity macros where even if my verbosity is explicitly set to 0 or 1, but my log level is Info, then a message like:

@errorv 2 "more detailed error message than you normally want to see"

will still be logged, since the current logic just does Error - 2.

With the change proposed in this PR, we get the following change in behavior:

  • If LoggingExtras.withlevel or an alternative filter doesn't inspect the group.verbosity argument, then the verbose macros act just like the normal logging macros
  • If withlevel is used, then the above case acts as expected; i.e. the @errorv 2 msg case only logs if withlevel(Info; verbosity=2) is set.

I opted to wrap the verbosity level in a new LoggingExtras.Verbosity struct so that if the group argument is ever used for something else as an Int, it won't accidentally conflict with the verbosity filtering. We'll still clobber that argument if anyone else tries to use it, but it shouldn't affect cases where people aren't using the verbosity macros.

Fixes JuliaWeb/HTTP.jl#938.

As noted in [this comment](https://github.com/JuliaLogging/LoggingExtras.jl/pull/64/files#r903295686),
there's currently a semantic error with the verbosity macros where even if my verbosity is
explicitly set to 0 or 1, but my log level is `Info`, then a message like:

```julia
@errorv 2 "more detailed error message than you normally want to see"
```

will still be logged, since the current logic just does `Error - 2`.

With the change proposed in this PR, we get the following change in behavior:
  * If `LoggingExtras.withlevel` or an alternative filter doesn't inspect
    the group.verbosity argument, then the verbose macros act just like
    the normal logging macros
  * If `withlevel` _is_ used, then the above case acts as expected; i.e.
    the `@errorv 2 msg` case only logs if `withlevel(Info; verbosity=2)`
    is set.

I opted to wrap the verbosity level in a new `LoggingExtras.Verbosity` struct
so that if the `group` argument is ever used for something else as an `Int`,
it won't accidentally conflict with the verbosity filtering. We'll still clobber
that argument if anyone else tries to use it, but it shouldn't affect cases where
people _aren't_ using the verbosity macros.

Fixes JuliaWeb/HTTP.jl#938.
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

I am convinced.
I am not sure why i was so opinionated the other way before.

I mean i do still see merit to my argument, but not as much.

(Aside: I continue to thinkg that the group should be something that acts kind of like an AbstractSet, so you can add multiple things and then check if any things are contained in it. But that is orthogonal to this PR)

I only have one small comment, (applies to multiple bits of the code)
address that as you see fit and merge when happy.

I think lets not tag a release with this, since as you say it is breaking.
but with this done we will be able to tag 1.0
(once we also do #70)

@quinnj quinnj merged commit 3b7ee61 into master Oct 21, 2022
@quinnj quinnj deleted the jq/verbosity_rework branch October 21, 2022 21:02
@oxinabox oxinabox mentioned this pull request Oct 28, 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.

Allow hiding handler error
2 participants