nrc / derive-new

derive simple constructor functions for Rust structs
MIT License
525 stars 35 forks source link

Make derive-new great again #18

Closed aldanor closed 7 years ago

aldanor commented 7 years ago

Hello and sorry for the massive PR, it was much easier in this case to do it all in one go rather than submit incremental patches.

Changes:

Basically, this now works :) (bikeshedding over attr syntax welcome)

#[derive(new, PartialEq, Debug)]
enum Enterprise<T: Default + PartialEq + Debug> {
    Picard,
    Data(
        #[new(value = "vec![-42, 42]")]
        Vec<i16>,
        #[new(default)]
        T,
    ),
    Spock {
        x: PhantomData<T>,
        y: i32,
    },
}

let x = Enterprise::<u8>::new_picard();
assert_eq!(x, Enterprise::Picard);

let x = Enterprise::<u8>::new_data();
assert_eq!(x, Enterprise::Data(vec![-42, 42], 0));

let x = Enterprise::<u8>::new_spock(42);
assert_eq!(x, Enterprise::Spock { x: PhantomData, y: 42 });

TODO, in separate PRs:

@nrc @dtolnay @kirillkh

nrc commented 7 years ago

Wow, that is a lot of commits, thanks!

So, I'll need to work through most of these, but they look good at first blush.

One comment on the PhantomData commit - I wonder if it would be better to have a new(ignore) attribute rather than special-casing PhantomData? It would be more boilerplate, but it is a more general-purpose mechanism. And it means you don't need to deal with scoping, hygiene, etc.

dtolnay commented 7 years ago

FWIW serde recognizes PhantomData as a special case here the same way and nobody has complained.

aldanor commented 7 years ago

Yea, the part with PhantomData is a tad hacky but would work in like 99% common cases.

#[new(ignore)] we could add separately too, I'm not sure there's much value in that though (since you still need to construct the initialiser value, I guess it has to be something like a unit struct, which is not the most common thing of all...)

aldanor commented 7 years ago

(Updated the README / description, not ideal but something's better than nothing)

nrc commented 7 years ago

OK, seems fine to special case PhantomData and perhaps support new(ignore) later. The README changes look good. So, I think if you can restore the Cargo.lock, then I can merge. Thanks!

aldanor commented 7 years ago

I've checked the updated Cargo.lock back in.

I also have some compile-fail tests in my local branch (for attr stuff), wondering if I should push them here as well before this gets merged?

nrc commented 7 years ago

I also have some compile-fail tests in my local branch (for attr stuff), wondering if I should push them here as well before this gets merged?

Sure, the more tests the better :-)

aldanor commented 7 years ago

@nrc Done :)

(btw it'd be nice to set up travis for all this of course, eventually...)

nrc commented 7 years ago

Thank you!