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

[WIP] Implement proof-of-concept of an enum "affix" #79

Merged
merged 3 commits into from
Aug 18, 2019
Merged

[WIP] Implement proof-of-concept of an enum "affix" #79

merged 3 commits into from
Aug 18, 2019

Conversation

ErichDonGubler
Copy link
Contributor

It'd be really nice to be able to derive Display and use an affix (prefix and/or suffix) that comes before each inner implementation of Display. This allows one to write a derived implementation of Display like so:

    // Expects a single Display item corresponding to where an inner 
    // variant's Display will be written (`{}`), otherwise errors
    #[derive(Debug, Display)]
    #[display(affix = "unable to read from /proc/stat: {}")]
    pub(crate) enum SnapshotReadError {
        Io(IoError),
        #[display(fmt = "expected CPU stat line for {}, got {:#?}", expected_cpu, line)]
        UnableToParseLine {
            expected_cpu: CpuComponent,
            line: String,
        }
    }

    #[derive(Debug, Display)]
    pub(crate) enum CpuComponent {
        #[display(fmt = "entire CPU")]
        EntireCpu,
        #[display(fmt = "CPU core {}", _0)]
        Core(usize),
    }

This can be particularly handy for error handling, but I'm sure there are other places this could be applied as well. :)


Things to do before this should be merged:

  • Design feedback: Mostly, is this interesting?
  • This commit probably shouldn't make things as hard to follow as it does -- how could this improve?
  • I specifically chose a terrible but concise word for this which I fully expect to be bikeshedded. Alternatives might include:
    • outer_fmt (I like this one the best.)
    • prefix (but it's not just a prefix!)
    • surround

@JelteF
Copy link
Owner

JelteF commented Aug 12, 2019

  1. I think this is a nice addition
  2. It's not too crazy, sadly code generation code is in generally quite hard to follow.
  3. If possible it would be very nice if we could simply use fmt. This would require a bit of changes to your code though, but I think it might actually make it simpler. If this is not possible I agree that outer_fmt is the best choice.

@ErichDonGubler ErichDonGubler changed the title [WIP] Implement proof-of-concept implementation of an enum "affix" [WIP] Implement proof-of-concept of an enum "affix" Aug 13, 2019
@JelteF JelteF merged commit d8de4b8 into JelteF:master Aug 18, 2019
@JelteF
Copy link
Owner

JelteF commented Aug 18, 2019

I changed the PR such that you can use fmt this way. It's on master now.

@ErichDonGubler ErichDonGubler deleted the affix branch August 26, 2019 22:31
@JelteF
Copy link
Owner

JelteF commented Nov 11, 2019

This is now released as version 0.99.0. Announcement link: https://reddit.com/r/rust/comments/duptc0/derive_more_0990_released_biggest_release_ever

@ErichDonGubler
Copy link
Contributor Author

@JelteF: After using this in production for some time, I want to submit that an affix key (as originally proposed in the OP I wrote) was probably the right design in the first place. A year after this gets merged, I came back to use a top-level enum format, something of the form:

use derive_more::Display;

#[derive(Debug, Display)]
#[display(fmt = "{:?}", self)]
enum Enum {
    Variant1,
    // ...
}

...but was confused by the diagnostic given (which I've filed a PR for) about a single fmt needing a single argument. It was only after examining source that I remembered that a top-level #[display(fmt = ..., ...)] on an enum uses an "affix mode". It's not very clear that suddenly expectations have changed, and it also makes the (somewhat intuitive, I think) case I was trying to implement above impossible without just manually impling the desired std::fmt trait again. Furthermore, I think it makes much more sense to prefer the above behavior for an outer enum fmt string, since that's much closer to what is already possible for

I'll filed a new issue proposing a breaking change for this use case with the above information, so let's continue discussion there.

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