-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
zv: Add missing From implementations for Value::Str #1238
Conversation
This fixes Value not implementing From for Arced and Cowed strs, even though they are implemented in zvariant::Str. This makes it possible to call .into() on those types and to create Arrays or Dicts without manually mapping the values. Fixes dbus2#1234
This replaces the manual implementation of From<String> and From<&String> in Value with the into_value_from_both! macro implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much! I don't understand how it's "non-atomic". You nicely split each logical change into separate commits and that's exactly what atomic commits is all about.
@@ -49,6 +49,9 @@ into_value_from_both!(i64, I64); | |||
into_value_from_both!(f32, F64); | |||
into_value_from_both!(f64, F64); | |||
|
|||
into_value!(Arc<str>, Str); | |||
into_value!(Cow<'a, str>, Str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for future ref, this is not bug fix but rather a new feature (despite its importance) so ✨ would have been a better emoji. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, thanks :)
I was referring to the PR as a whole, since I was committing something that didn't have much to do with what the PR was trying to address. |
Ah ok. While smaller PRs are usually better, I don't think it's important to keep PRs completely atomic. In fact splitting PRs too much can cause a lot of noise in the git history for very little gain. Atomic commits OTOH have real benefits (every change is documented, easy to bisect when hunting a regression down, easy to revert changes etc). Hence why I only stress on atomic commits. |
This PR mainly fixes Value not implementing From for Arced or Cowed strs.
Additionally I replaced the manual implementations of
From<String>
andFrom<&'v String>
with the macro one.I think this makes the PR not atomic anymore, sorry about that. 😅
fixes #1234