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

Implement server side named parameters #634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luc65r
Copy link

@luc65r luc65r commented Aug 26, 2021

No description provided.

@luc65r luc65r force-pushed the master branch 2 times, most recently from 36a3803 to 03b0feb Compare August 26, 2021 19:32
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome stuff!! Looks great, thanks!

@luc65r
Copy link
Author

luc65r commented Aug 29, 2021

Oh I just found out it doesn't work with the meta argument

@tomusdrw
Copy link
Contributor

Good point, didn't think of that. Can we add a test for this? I guess the solution would be to either:

  1. Attempt to detect the meta argument (either based on the type or the variable name)
  2. Require an extra attribute/variant to skip the first parameter.

@luc65r
Copy link
Author

luc65r commented Aug 30, 2021

I fixed it, I hope I didn't change too much code.

Now it has some issues with attributes (see commented out code in tests/macros.rs). Attributes can be in an other pr.

derive/src/to_delegate.rs Outdated Show resolved Hide resolved
Comment on lines 239 to 240
let param_types: Vec<_> = fn_args.iter().cloned().map(|arg| *arg.ty).collect();
let arg_names: Vec<_> = fn_args.iter().cloned().map(|arg| *arg.pat).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let param_types: Vec<_> = fn_args.iter().cloned().map(|arg| *arg.ty).collect();
let arg_names: Vec<_> = fn_args.iter().cloned().map(|arg| *arg.pat).collect();
let param_types = fn_args.iter().map(|arg| *arg.ty).collect::<Vec<_>>();
let arg_names = fn_args.iter().map(|arg| *arg.pat).collect::<Vec<_>>();

My preference would be to add a generic parameter to collect, but I'm not super strong on this.

Also .cloned() doesn't seem required if we dereference/copy the type, is it?

Copy link
Author

Choose a reason for hiding this comment

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

syn::Type and syn::Pat don't implement Copy, so there must be a .clone() somewhere.
It should be more efficient to clone only syn::Type or syn::Pat instead of the whole syn::FnArg.

derive/src/to_delegate.rs Outdated Show resolved Hide resolved
derive/src/to_delegate.rs Show resolved Hide resolved
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