Closed KodrAus closed 7 months ago
We currently have kv::source::Visitor
and kv::value::Visit
. They're both visitor types (separate because they serve separate purposes), but should probably still be named consistently. @Thomasdezeeuw what do you think? Should we rename either source::Visitor
or value::Visit
? My preference would be to rename value::Visit
to value::Visitor
.
We currently have
kv::source::Visitor
andkv::value::Visit
. They're both visitor types (separate because they serve separate purposes), but should probably still be named consistently. @Thomasdezeeuw what do you think? Should we rename eithersource::Visitor
orvalue::Visit
? My preference would be to renamevalue::Visit
tovalue::Visitor
.
I think Visitor
is more common? So I would go with that.
I would understand possible confusing between the two traits, so maybe we need to rename one of the two? What about renaming source::Visitor
to KeyValuePairs
or just KeyValues
, to indicate its a collection of key-value pairs?
I would understand possible confusing between the two traits, so maybe we need to rename one of the two?
That's a fair point. Perhaps we could keep the names verb-y and use VisitSource
and VisitValue
?
That's a fair point. Perhaps we could keep the names verb-y and use
VisitSource
andVisitValue
?
Sounds good :+1:
Ok, I think the last remaining thing here is macro syntax. We could adopt exactly tracing
's syntax of:
key = <modifier> arg
where <modifier>
may be ?
for fmt::Debug
or %
for fmt::Display
. I'm not a huge fan of leaning on sigils, or on prefixed modifiers here, because it leaves us almost no room for extensibility, but does have the benefit of being what's already there. I was thinking something like:
key = arg : <modifier>
where <modifier
> may be blank for fmt::Display
, ?
for fmt::Debug
, or the literal text serde
or sval
for those respective libraries. In practice, it would look something like:
data
: captured using ToValue
data:
: captured using fmt::Display
data:?
: captured using fmt::Debug
data:serde
: captured using serde::Serialize
data:sval
: captured using sval::Value
We could use a simple lowering scheme so data:?
lowered to data:debug
and data:
lowered to data:display
, so it would be less easy to miss the presence of the :
.
Another potential position could be on the key name:
key : <modifier> = arg
That would more easily let us expected arg
to be a full expression in the future.
Any thoughts?
key = arg : <modifier>
where
<modifier
> may be blank forfmt::Display
,?
forfmt::Debug
, or the literal textserde
orsval
for those respective libraries. In practice, it would look something like:
I like this, feels similar to write!
(and print!
, etc.) macros.
* `data:serde`: captured using `serde::Serialize` * `data:sval`: captured using `sval::Value`
Small thing is versioning, in case serde/sval ever publish a v2, how do we want to handle that? We can go serdeV1
or serde1
or something else with a version in it, but if a v2 never comes it will just be annoying. So maybe serde
/sval
means v1, and serde2
/sval2
could v2?
We could use a simple lowering scheme so
data:?
lowered todata:debug
anddata:
lowered todata:display
, so it would be less easy to miss the presence of the:
.
Because we'll be copying the standard formatting found in the std lib, I don't think will become an issue.
Another potential position could be on the key name:
key : <modifier> = arg
That would more easily let us expected
arg
to be a full expression in the future.
How would be limiting ourselves using the above arg : <modifier>
? I suppose we won't be able to support full expressions?
So maybe serde/sval means v1, and serde2/sval2 could v2?
That seems like a good scheme to me :+1: I spelled this out explicitly in value-bag
, but it's not really meant to be user-facing. I think it's reasonable to make the "unversioned" ident refer to the current stable versions in log
.
How would be limiting ourselves using the above arg :
? I suppose we won't be able to support full expressions?
We'd just need to wrap full expressions in parenthesis or braces, because the :
character can't appear directly after an expression in a macro rule.
This is quite a big change but I think it's ready for a review now. Some things to call out:
key:modifier = value
, so key:? = value
for debug for instance. This was easier to implement and didn't require breaking any existing users.kv::source
and kv::value
modules private now there's no types in them that aren't exported in the root kv
module? I think we probably should.The
err:error = err
way of capturing an error using the std::error::Error
trait is maybe something we can improve on someday 🙂
I think the last thing remaining here is what to do about the _unstable
suffix on our features. I was thinking making them just shims for the unsuffixed versions, but continuing to accept them so that we don't break every current user of the feature. What do you think @Thomasdezeeuw?
How does this sound as a plan for stabilization:
kv_unstable
feature is enabled, deprecating wherever possible.kv_unstable
feature altogether and all those deprecated items. If we keep rough tabs on public code using the old features then we could get some idea of when would be a good time to do this.The idea being new code and any maintained in that time period should move from the unstable to the stable API without much fuss, but there will be a point where the bandaid comes off completely. The only unfortunate part is we can’t deprecate old trait names so we don’t have a good way of making users of them aware things have changed.
How does this sound as a plan for stabilization:
Sounds good to me :+1:
In this latest round of changes I've restored the old APIs under their respective *_unstable
feature flags and added support for shorthand capturing, so instead of having to write info!(a = a; "")
you can write info!(a; "")
.
Ok, I think this is ready for its last review now. Thank you so much for all the time you've invested in helping push this forwards so far @Thomasdezeeuw 🙏 We'd never have reached this point without you.
Closes #149 Closes #328 Closes #357 Closes #436 Closes #584
Based on the review in #584
The plan is to:
Value::capture_*
andValue::downcast_*
methods. All deprecated APIs are gated under the oldkv_unstable
features and will be removed once we remove those old unstable features.ToValue
(key:? = value
).value-bag
as a dependency if eitherserde
orsval
is also needed. In cases where you just want basic structured logging, we should do it in a dependency-free way. This will just mean adding a simpleValueInner
enum.I'll cc once this is ready for review, to save a bit of notification noise.
Once this PR is complete, we should be able to stabilize structured logging support in the
log
crate.