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

Delegation for fmt traits (#321) #322

Merged
merged 28 commits into from
Dec 22, 2023
Merged

Delegation for fmt traits (#321) #322

merged 28 commits into from
Dec 22, 2023

Conversation

tyranron
Copy link
Collaborator

@tyranron tyranron commented Dec 15, 2023

Resolves #321

Synopsis

See #321 (comment):

You’re discarding formatting flags provided by the user in format string, e.g.:

#[derive(derive_more::Display)]
#[display(fmt = "{:?}", _0)]
struct Num(usize);

impl std::fmt::Debug for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        self.0.fmt(fmtr)
    }
}

fn main() {
    let num = Num(7);
    println!("{num:03?}");  // prints ‘007’ as expected
    println!("{num:03}");   // prints ‘7’ instead
}

Solution

See #321 (comment):

Theoretically, we can support this with the current syntax, because we can detect so-called trivial cases and transform them into delegation (we do parse formatting string literal anyway).

#[derive(derive_more::Display)]
#[display("{_0:?}")] // <--- it's clear to be a trivial delegation case
struct Num(usize);

would expand to

impl std::fmt::Display for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        let _0 = &self.0;
        std::fmt::Debug::fmt(_0, fmtr)
    }
}

rather than

impl std::fmt::Display for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        let _0 = &self.0;
        write!(fmtr, "{_0:?}")
    }
}

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

@tyranron tyranron added this to the 1.0.0 milestone Dec 15, 2023
@tyranron tyranron self-assigned this Dec 15, 2023
@tyranron tyranron marked this pull request as ready for review December 18, 2023 19:01
@tyranron tyranron requested a review from JelteF December 18, 2023 19:01
impl/doc/display.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
impl/src/fmt/mod.rs Outdated Show resolved Hide resolved
impl/src/fmt/display.rs Outdated Show resolved Hide resolved
impl/src/fmt/debug.rs Outdated Show resolved Hide resolved
@tyranron tyranron marked this pull request as draft December 19, 2023 11:46
@tyranron tyranron marked this pull request as ready for review December 20, 2023 18:15
@tyranron tyranron requested a review from JelteF December 20, 2023 18:15
.spec
.map(|s| s.ty)
.unwrap_or(parsing::Type::Display)
.trait_name();
Copy link
Owner

Choose a reason for hiding this comment

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

When is this "default" case applicable?

Copy link
Collaborator Author

@tyranron tyranron Dec 21, 2023

Choose a reason for hiding this comment

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

@JelteF when format_spec is not specified, i.e. {} or {ident}. See grammar for details.

Comment on lines +237 to +240




Copy link
Owner

@JelteF JelteF Dec 21, 2023

Choose a reason for hiding this comment

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

were all these newlines intentional? (feel free to keep if they were)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JelteF yes.... 4 newlines before ## headers and 2 newlines before ### headers.

Comment on lines +156 to +157


Copy link
Owner

Choose a reason for hiding this comment

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

were all these newlines intentional? (feel free to keep if they were)

Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the extensive test suite! :shipit:

@tyranron tyranron merged commit 732bb12 into master Dec 22, 2023
16 checks passed
@tyranron tyranron deleted the fmt-delegation branch December 22, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Add #[display(delegate)] option for newtypes which delegates to wrapped type
2 participants