Add support for labelling TupleStructs by index #144
Conversation
| fn build_new_labelled_tuple_struct_constr( | ||
| struct_name: &Ident, | ||
| bindnames: &Vec<Ident>, | ||
| ) -> Box<dyn ToTokens> { |
There was a problem hiding this comment.
Is there a reason why impl ToTokens doesn't work here?
There was a problem hiding this comment.
These return types have to unify in the match statement when they're called, and impl Trait return values cannot unify (except with themselves).
By returning a trait object, these two return types can be used interchangeably.
There was a problem hiding this comment.
Ah, the match expression is somewhere else... OK, let's move the Box::new(...) to that place instead and write -> impl Trait here so that the reason for the dynamic dispatch is clearer and so that it is done where it is actually needed.
| fn build_new_labelled_struct_constr( | ||
| struct_name: &Ident, | ||
| bindnames: &Vec<Ident>, | ||
| ) -> Box<dyn ToTokens> { |
|
Markups applied |
|
Okay; supporting this makes sense from the standpoint of generic interfaces (i.e. writing a function that uses the semantics of Should we similarly impl There is a question here of whether tuple structs should use underscore-prefixed names or not.
However, the difference may be moot, because ISTM there are vanishingly few places that can make use of either of these advantages. (these differences sound like they would be useful mostly in codegen, but codegen cannot make use of type information) |
|
Unit test - Yes, sure, there are no tests in the derive crate, where's the best place to test the derive code?
LabelledGeneric for Tuples? Seems reasonable, does that block this PR? |
|
I think tests for derives are in `frunk/tests` as integration tests (you
can't unit test a proc macro from within it's own crate)
…On Tue, Jan 29, 2019, 2:59 PM Andy Caldwell ***@***.*** wrote:
Unit test - Yes, sure, there are no tests in the derive crate, where's the
best place to test the derive code?
Field names - I'll admit I didn't think too hard about this... I went for
_x since they're valid identifiers, but this does allow transmogrifying
between Foo(String) and Bar { _0: String } which is a little surprising.
Since it's not possible to define Bar { 0: String } the confusion
disappears, but this precludes conversion from named structs to tuple
structs forever...
LabelledGeneric for Tuples? Seems reasonable, does that block this PR?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#144 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABWI0HYlJuRALjlrepeAvpdCqg39jgA3ks5vIKgYgaJpZM4aM_6f>
.
|
Sorry yeah, I meant integration tests under /tests and/or doc tests ( I think they work in there for some reason ?). https://docs.rs/frunk/0.2.2/frunk/
Good point; that is a bit surprising and yet .. precluding conversion from named structs to tuple structs might not be desirable? One could make the argument that if someone is naming their field |
|
Okay, it's hard to predict which is the better choice. Since nobody seems to have a strong opinion the current implementation is probably fine for now. Re: tuple impl, I don't think we need to block on it. I do think it should be added before the next release though. |
|
Oh: add a note to the release notes under "Unreleased" |
|
I've added a couple of UTs (one for the use case I care about, that of new-type wrappers being interchangeable if the inner type is the same), and added an entry to the changelog. |
|
Released as 0.2.4 thanks for the great work 😄 |
This is a little unusual, since it basically undoes the good that
LabelledGenerics by allowing someone to transmute in a way that switches the meaning of two members of the tuple but:LabelledGenericsand still transmogrify freely.LabelledGenericis opt-in so if there's a struct that's too primitive for transmogrifying to make sense (e..gPair<T>(T, T)) then don't opt-in.