-
-
Notifications
You must be signed in to change notification settings - Fork 85
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,zn: manually implement Debug for Str, *Name and ObjectPath #394
Conversation
As a follow-up PR, I think
|
Nice but:
|
Could you explain what you mean? |
Good that you asked. I totally missed how that both outputs are of |
Actually I think it's better to achieve this by manually implementing |
Sure.
I thought so. Was just linking the PRs for their close similarity. |
@MaxVerevkin btw, did I mention our newest strong recommendation to have emoji prefixes on git commits? They also make for good release notes: https://github.com/dbus2/zbus/releases/tag/zvariant-3.15.0 (I also rollod out zbus_names release but most commits there were lacking the emojis so the release notes don't look at good). I added recently to our contributions guide, with recommendations for tools to use for doing it easily. |
No, I noticed the use of emojis in the git log, but I wasn't aware it is a strong recommendation to use them. Personally not a fan, but I'll add them to my commit messages. |
I think it adds more than just aesthetics. It's easier to browse through the history with them. A picture is worth a thousand words they say. :)
Thank you! 🙏 If nothing else than you'll be doing it for consistency. Maybe you'll learn to like them, who knows. :) |
38a30e7
to
a3889ee
Compare
I also use vim so this affects me too, just that I never used that emoji. :) A couple of things:
|
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.
Mostly just commit message improvements left. LGTM otherwise.
@@ -120,6 +120,12 @@ impl Display for BusName<'_> { | |||
} | |||
} | |||
|
|||
impl Debug for BusName<'_> { |
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.
The commit log (manually implement Debug for *Name
) doesn't mention the rationale. Manual impl is an (no pun intended) implementation detail, not the summary of the change. How about:
zn: Improved debug repr for *Name
Improve debug representation of *Name through a manual impl of `Debug`.
Before:
<output>
After:
<output>
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.
similar for the other Debug impl commits.
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.
I think "transparent impl" is a good term to explain this change?
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.
Perhaps? or go with my suggestion.
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.
Should I repeat it in each commit msg?
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.
Should I repeat it in each commit msg?
repeating is fine (atomic commits).
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.debug_tuple("ObjectPath").field(&self.as_str()).finish() | ||
Debug::fmt(self.as_str(), f) |
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.
Would be nice to document briefly in the commit description, what was removed.
@MaxVerevkin is this still Draft? |
It is draft is a sense that I haven't addressed review feedback. I'll try to do it soon. |
@SeaDve seems @MaxVerevkin doesn't have time to finish this. Feel free to do that in a separate PR if you want to work on this. Since git doesn't allow multiple authors, when you modify a commit you can either:
Based on how much you had to modify the commit. |
@SeaDve do you think you'll have time to do this soon? |
Possibly on friday or saturday this week |
I opened a separate PR with different implementation |
closing in favour of #450 |
Before:
After:
Fixes #431.