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

Change Debug to use string representation #4

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Conversation

cortopy
Copy link
Contributor

@cortopy cortopy commented Jun 23, 2022

Hi! I've been using xid for a few months and it works great, thanks for this!

However, I find myself struggling now that I've started debugging my program more often. Even though Id implements debug, it prints the raw bytes, which is not human-friendly at all when debugging and creates a lot of noise.

This PR just adds a manual implementation so that it prints the string

@kazk
Copy link
Owner

kazk commented Jun 24, 2022

Hi! Thanks for the PR.

fmt::Debug is for showing the internals, and changing it to a string representation will make that impossible. Display is for a human-friendly output. I didn't realize Id didn't have impl Display 🤦‍♂️, so I added it in #5 and released v1.0.1.

@kazk kazk closed this Jun 24, 2022
@cortopy
Copy link
Contributor Author

cortopy commented Jun 24, 2022

Debug as it is would be helpful for developing this crate itself, but a type like Id will most likely be part of a struct (e.g.: a User, a Purchase, etc.) which when debugging will be impossible to know which one it is by looking at the logs.

From the point of view of the consumer of this library, a stringified representation seems to make more sense. Indeed, libraries like uuid is what they do for both Display and Debug

@kazk
Copy link
Owner

kazk commented Jun 24, 2022

a type like Id will most likely be part of a struct (e.g.: a User, a Purchase, etc.) which when debugging will be impossible to know which one it is by looking at the logs.

This crate doesn't have serde feature yet because I haven't needed it. It can be convenient to have deserialization to fail when an invalid id is found, but storing it as a string has been enough for my use cases. If you want it, I'll accept a PR.

The structs can implement custom Debug, too.

UUID's string representation is lower-hexed bytes, so it doesn't sacrifice anything.

That said, I agree it'll be convenient for the consumers. And I actually don't think keeping the current Debug will be that useful anyway.

@kazk kazk reopened this Jun 24, 2022
@@ -84,6 +84,12 @@ impl ToString for Id {
}
}

impl std::fmt::Debug for Id {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_tuple("Id").field(&self.to_string()).finish()
Copy link
Owner

Choose a reason for hiding this comment

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

Id("9m4e2mr0ui3e8a215n4g") looks like a newtype for string. How about this?

impl fmt::Debug for Id {
    #[inline]
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "xid:{}", self)
    }
}
println!("{:?}", id); // xid:9m4e2mr0ui3e8a215n4g

You'll need to rebase this to get Display.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Id("9m4e2mr0ui3e8a215n4g") looks like a newtype for string. How about this?

impl fmt::Debug for Id {
    #[inline]
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "xid:{}", self)
    }
}
println!("{:?}", id); // xid:9m4e2mr0ui3e8a215n4g

You'll need to rebase this to get Display.

My first reaction was to think "is this Rust idiomatic?". In my experience all the tuple structs I've seen with Debug implemented use the format Name(value). To me this makes it apparent what it is, and what value it has.

Having something like xid:{} would suggest that if you do id.to_string() that's also what you get since the Debug printed value looks like a string after all?

Copy link
Owner

Choose a reason for hiding this comment

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

My first reaction was to think "is this Rust idiomatic?"

I think idiomatic Rust is deriving Debug or custom Debug with similar output, and showing the internals. In this case, we decided not to for consumer's convenience, so we should optimize for that.

In my experience all the tuple structs I've seen with Debug implemented use the format Name(value). To me this makes it apparent what it is, and what value it has.

That's the problem I mentioned above. Id("9m4e2mr0ui3e8a215n4g") is what you get for

#[derive(Debug)]
struct Id(String);

which is misleading.

Having something like xid:{} would suggest that if you do id.to_string() that's also what you get since the Debug printed value looks like a string after all?

I don't think so? I'm fine without the prefix (just Display) if that bothers you.

@kazk kazk changed the title debug as string Change Debug to use string representation Jun 24, 2022
@cortopy
Copy link
Contributor Author

cortopy commented Jun 25, 2022

a type like Id will most likely be part of a struct (e.g.: a User, a Purchase, etc.) which when debugging will be impossible to know which one it is by looking at the logs.

This crate doesn't have serde feature yet because I haven't needed it. It can be convenient to have deserialization to fail when an invalid id is found, but storing it as a string has been enough for my use cases. If you want it, I'll accept a PR.

The structs can implement custom Debug, too.

UUID's string representation is lower-hexed bytes, so it doesn't sacrifice anything.

That said, I agree it'll be convenient for the consumers. And I actually don't think keeping the current Debug will be that useful anyway.

Thanks so much for reconsidering this! Yes, serde has human friendly formatting (which normally is associated with Debug) as a very explicit way of dealing with this.

Having said that I'm not using Serde but minicbor. I'm storing xid as bytes because minicbor encodes full structs to bytes in my program. Hence, my difficulties in debugging with debug, as the whole bytes array is shown

Copy link
Owner

@kazk kazk left a comment

Choose a reason for hiding this comment

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

I decided to merge this as is, before I forget. It's honestly not a big deal. I don't really need this either, and it works for you.

We can change this later if someone thinks it's misleading.

@kazk kazk merged commit 138eca8 into kazk:main Jul 5, 2022
kazk added a commit that referenced this pull request Jul 5, 2022
- Change `Debug` for `Id` to use the string representation (#4)
@cortopy
Copy link
Contributor Author

cortopy commented Jul 22, 2022

@kazk I'm so sorry I didn't reply earlier, this somehow fell through. Thank you for approving this!

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