-
Notifications
You must be signed in to change notification settings - Fork 770
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
add better error message for Python in signature #2930
Conversation
"expected argument from function definition `{}` but got argument `{}`", | ||
fn_arg.name.unraw(), | ||
name.unraw(), | ||
struct ArgsValidator<'b, 'a: 'b>(std::slice::IterMut<'b, FnArg<'a>>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the change from closure to named type required for this to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplest way I could see to implement this was to use recursion to skip over Python
arguments, which makes a horrible mess of closures.
Not sure what I've come up with here is the prettiest either. Maybe give me another shot at this tomorrow and I'll try factor it out differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yes recursion and closure would indeed be a mess. I am not opposed to the change, I just asked to make sure that I did not miss anything. (And indeed I did not see the recursion at a first glance.)
For such cases, I think loop
might help, i.e. turn
fn func(iter: I) -> R {
if let Some(val) = iter.next() {
if condition(val) {
return func(iter);
}
...
} else {
...
}
}
into
fn func(iter: I) -> R {
loop {
if let Some(val) = iter.next() {
if condition(val) {
continue;
}
break ...;
} else {
break ...;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be missing something, but this looks like an awful way to reinvent retain
.
let mut err = None;
arguments.retain(|a| /* delete some things and/or set err */);
if let Some(err){
return ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly - we don't want to remove elements from arguments
, just enrich them based off the attribute information. This might be able to be refactored better once deprecated #[args]
is removed.
I'll have another go at tidying this up now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A for
loop is certainly nicer than an open-coded loop
loop. LGTM.
(The .by_ref()
is required because adding move |...
to the closure would move more than just the iterator into it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .by_ref()
is needed because I don't want to move the iterator into the closure - I check at the end of the function that we've consumed everything from it.
17a4ff1
to
b5dfe14
Compare
b5dfe14
to
96efb0e
Compare
bors r=adamreichold |
Build succeeded: |
Inspired by #2929, this just adds a better error message when
Python
arguments are accidentally included in the signature.