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

core: add conversion from Option<Level> to LevelFilter #966

Merged

Conversation

davidbarsky
Copy link
Member

This PR resolves a previously unreported regression where Option<Level> was no longer a valid LevelFilter. Below is a minimal reproduction of the issue:

tracing_subscriber::fmt().with_max_level(Some(tracing::Level::WARN)).init();

I've added an From<Option<Level>> for LevelFilter implementation which might require an MSRV version bump to 1.41 to support the rebalanced coherence changes.

@davidbarsky davidbarsky requested a review from a team September 5, 2020 17:28
@hawkw
Copy link
Member

hawkw commented Sep 5, 2020

which might require an MSRV version bump to 1.41 to support the rebalanced coherence changes.

I don't think that's the case --- it appears to compile on 1.40.0. The From impl is for a type that's defined in the current crate, so there's no potential coherence issue. :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

The implementation looks fine. It seems like we accidentally removed the Into<Option<Level>> impl, can we fix that before merging this?

Also, it might be nice to add examples of the Option<Level> conversions in the docs for LevelFilter? Not a blocker for the fix though.

tracing-core/src/metadata.rs Show resolved Hide resolved
tracing-core/src/metadata.rs Outdated Show resolved Hide resolved
@davidbarsky
Copy link
Member Author

I don't think that's the case --- it appears to compile on 1.40.0. The From impl is for a type that's defined in the current crate, so there's no potential coherence issue. :)

Nice! I was worried about the Option in the bounds, but you're right. If I understand the rebalancing coherence RFC correctly, we'd only run into the issue the RFC seeks to address if there are generics in impl clause, which there are not.

@hawkw
Copy link
Member

hawkw commented Sep 5, 2020

I don't think that's the case --- it appears to compile on 1.40.0. The From impl is for a type that's defined in the current crate, so there's no potential coherence issue. :)

Nice! I was worried about the Option in the bounds, but you're right. If I understand the rebalancing coherence RFC correctly, we'd only run into the issue the RFC seeks to address if there are generics in impl clause, which there are not.

Yup!

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm

@davidbarsky davidbarsky merged commit 11f577a into master Sep 5, 2020
@davidbarsky davidbarsky deleted the davidbarsky/fix-missing-levelfilter-from-conversion branch September 5, 2020 20:16
hawkw pushed a commit that referenced this pull request Sep 8, 2020
# 0.1.16 (September 8, 2020)

### Fixed

- Added a conversion from `Option<Level>` to `LevelFilter`. This
  resolves a previously unreported regression where `Option<Level>` 
  was no longer a valid LevelFilter. (#966)
hawkw added a commit that referenced this pull request Sep 8, 2020
Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be
  enabled when multiple subscribers are in use ([#927])

Changed

- **json**: `format::Json` now outputs fields in a more readable order
  ([#892])
- Updated `tracing-core` dependency to 0.1.16

Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support
  libtest output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, and
@SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
hawkw added a commit that referenced this pull request Sep 9, 2020
# 0.2.12 (September 8, 2020)

### Fixed

- **env-filter**: Fixed a regression where `Option<Level>` lost its
  `Into<LevelFilter>` impl ([#966])
- **env-filter**: Fixed `EnvFilter` enabling spans that should not be enabled
  when multiple subscribers are in use ([#927])
  
### Changed

- **json**: `format::Json` now outputs fields in a more readable order ([#892])
- Updated `tracing-core` dependency to 0.1.16

### Added

- **fmt**: Add `BoxMakeWriter` for erasing the type of a `MakeWriter`
  implementation ([#958])
- **fmt**: Add `TestWriter` `MakeWriter` implementation to support libtest
  output capturing ([#938])
- **layer**: Add `Layer` impl for `Option<T> where T: Layer` ([#910])
- **env-filter**: Add `From<Level>` impl for `Directive` ([#918])
- Multiple documentation fixes and improvements

Thanks to @Pothulapati, @samrg472, @bryanburgers, @keetonian, 
and @SriRamanujam for contributing to this release!

[#927]: #927
[#966]: #966
[#958]: #958
[#892]: #892
[#938]: #938
[#910]: #910
[#918]: #918
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.

None yet

2 participants