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.

Sign up for GitHub

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

Jump to bottom

Implement FromTypeParam #30

Merged
merged 2 commits into from
May 3, 2018
Merged

Implement FromTypeParam #30

merged 2 commits into from
May 3, 2018

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Apr 30, 2018

This fixes #28.

@upsuper
Copy link
Contributor Author

upsuper commented Apr 30, 2018

(Actually I have a feeling that lots of code can be reused in this crate...)

TedDriggs
TedDriggs requested changes May 1, 2018
View reviewed changes
Copy link
Owner

@TedDriggs TedDriggs left a comment

Choose a reason for hiding this comment

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

This is looking good; thanks for submitting it! I've left a couple hopefully-small items. If you could look at those and consider adding a new example, I think we'll be good to go!

core/src/codegen/from_type_param.rs
pub attrs: Option<&'a Ident>,
pub attr_names: Vec<&'a str>,
pub forward_attrs: Option<&'a ForwardAttrs>,
pub from_ident: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

For the reason mentioned on #29, please use Option<bool> here.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll look at this after we merge.

tests/from_type_param.rs
}

#[test]
fn expand_many() {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a test with a defaulted type please?

@TedDriggs
Copy link
Owner

TedDriggs commented May 1, 2018 •
edited
Loading

One more thought: darling has some magic fields that make it easy to get a struct from DeriveInput. Should we have a way for the generics magic field to be populated with things that implement FromTypeParam? That might require a FromGenerics trait also be added, which is probably outside the scope of this MR.

@upsuper
Copy link
Contributor Author

upsuper commented May 1, 2018

That can be in a followup change. Also type param can have magic fields for things like bounds. And to implement FromGenerics, it would probably be good to also have FromConstParam and FromLifetimeDef.

@TedDriggs TedDriggs merged commit 9013579 into TedDriggs:master May 3, 2018
@upsuper upsuper deleted the from-type-param branch May 3, 2018 00:25
@TedDriggs TedDriggs mentioned this pull request May 3, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TedDriggs TedDriggs TedDriggs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add FromTypeParam
2 participants
@upsuper @TedDriggs