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

Respect fmt trait and flags in Value #400

Merged
merged 3 commits into from Jun 5, 2020

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Jun 4, 2020

Fixes #395

This PR makes the fmt::Debug and fmt::Display impls on Value respect the trait being used and any flags present. I've added a separate visitor for Display that uses the right trait, and pass the value directly to the formatter instead of going through format_args.

There are some extra tests for formatting and I've shifted a few pieces around to be consistent. Tests won't pass until I fix sval-rs/sval#84 as well.

r? @yoshuawuyts

@@ -107,48 +149,6 @@ impl Error {
}
}

struct SvalVisitor<'a, 'b: 'a>(&'a mut sval::value::Stream<'b>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved up into the impl block, like the visitors in the fmt module.

@@ -67,10 +67,4 @@ mod std_support {
Error::boxed(err)
}
}

impl From<Error> for io::Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We removed a similar external From impl in #398 that was causing problems with inference.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

💯 -- thanks heaps!

@KodrAus KodrAus merged commit 9ae6778 into rust-lang:master Jun 5, 2020
@KodrAus KodrAus deleted the fix/value-fmt branch June 5, 2020 11:21
Fishrock123 added a commit to Fishrock123/femme that referenced this pull request Nov 24, 2020
rust-lang/log#400
was merged upstream and caused this crate's json key-value logging to break.

This is problem for Tide, which uses key-value logging in it's default log middleware.

Fixes: lrlna#17
Refs: http-rs/tide#752
lrlna added a commit to lrlna/femme that referenced this pull request Dec 4, 2020
rust-lang/log#400
was merged upstream and caused this crate's json key-value logging to break.

This is problem for Tide, which uses key-value logging in it's default log middleware.

Fixes: #17
Refs: http-rs/tide#752

Co-authored-by: Irina Shestak <lrlna@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants