I don't believe ScopedLoggingContexts should obviously accept null "no-op" arguments.
- Dominant language
- Java
- Stars
- 1.5k
- Forks
- 133
- Avg merge
- 6m
- Merged PRs (30d)
- 7
Description
See:
https://github.com/google/flogger/commit/5aa06498bacb2790dde73fd4c30c9ac796b58047#commitcomment-124981853
The problem with the commit which added this is that now you can pass in null (perhaps via a variable) and get unexpected runtime failures when building a logging context.
```java
ScopedLoggingContext.newContext().withTags(tags1).withTags(tags2)...
```
Will fail at runtime if both tags1 and tags2 are non zero (the API only lets you see one tags instance). However if tags1 is null, the code will not throw an exception.
When I designed this API I had explicitly made it so that only one call withTags/withLogLevel was allowed (this encourages people to pull together they data they want to add before making the call). With the new change, this breaks that behaviour.
1. withTags() doesn't need to cope with a null parameter as a "no-op", since `Tags.empty()` exists.
2. withLogLevel() also has a valid "no-op" instance (via `create(Level.OFF)` which could be pulled out into a static field).
Having these methods accept null (and having that create different behaviour to passing the non-null no-op instance) is potentially confusing to users.
Only `withMetadata()` (which is defined to be able to be called more than once) arguably needs to handle null input, and that should just behave in exactly the same way as `with()` in the logging API.
Contributor guide
Assessment
This issue has not been assessed yet.