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

Debug impl for Decimal/Decimal256 uses decimal output #1600

Closed
wants to merge 2 commits into from
Closed

Debug impl for Decimal/Decimal256 uses decimal output #1600

wants to merge 2 commits into from

Conversation

snoyberg
Copy link
Contributor

No description provided.

@webmaster128
Copy link
Member

Good idea, thanks. Could you do the same change for Decimal and add tests for that (see e.g. fn binary_implements_debug() { for a quick example test)?

@snoyberg
Copy link
Contributor Author

Totally, complete oversight on my part about Decimal. I'll make both changes now.

@webmaster128 webmaster128 changed the title Debug impl for Decimal256 uses decimal output Debug impl for Decimal/Decimal256 uses decimal output Jan 26, 2023
@@ -457,6 +457,12 @@ impl fmt::Display for Decimal {
}
}

impl fmt::Debug for Decimal {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self)
Copy link
Member

Choose a reason for hiding this comment

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

Okay, now this shows the value but the Debug implementation should also include the type, right? Otherwise Decimal and Decimal256 cannot be differentiated. Also https://doc.rust-lang.org/std/fmt/trait.Debug.html suggests including the type name.

What about this?

Suggested change
write!(f, "{}", self)
write!(f, "Decimal({})", self)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, and I'd considered going in that direction originally. I didn't realize the Debug docs recommend including the type name, thank you for pointing that out! I've updated the PR with that change.

@webmaster128
Copy link
Member

Rebased and merged as part of #1602. Thank you!

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.

2 participants