-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement kwargs in function calls #115
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
Conversation
This doesn't enforce ordering of non-keyword and keyword arguments, and doesn't actually use the keywords (the arguments will still be passed through in order as positional).
@@ -137,7 +137,8 @@ pub enum Expression { | |||
}, | |||
Call { | |||
function: Box<Expression>, | |||
args: Vec<Expression>, | |||
// parameters are (None, value), kwargs are (keyword name, value) | |||
args: Vec<(Option<String>, Expression)>, |
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.
Are kwargs allowed before normal args? If not, I would suggest two vectors, args and kwargs. In any case I would like to have a seperate struct for kwargs with a name and expression member.
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.
They aren't, but these are created in the lalrpop-generated code. I couldn't work out a good way of expressing the ordering there, so at the moment, we enforce the ordering in the compiler.
The problem is, I think, that with foo(x, y="z")
the parser can't know when it examines y
whether or not it is a positional argument or a keyword argument.
My grammar-fu is very weak though, so there may well be a way of handling that!
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.
Okay, I'll have a look later on.
This would be cool, so we could implement |
vm/src/vm.rs
Outdated
.to_vec() | ||
.unwrap() | ||
.iter() | ||
.map(|pyobj| pyobj.to_str().unwrap()) |
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.
vm.to_str might be the better option here?
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.
These are the keyword argument names, which can only be literal strings:
$ python -c 'foo("s"+"a"="foo")'
File "<string>", line 1
SyntaxError: keyword can't be an expression
so we shouldn't ever get in to a case where this isn't a string; I've just pushed up a commit to use objstr::get_value
directly.
vm/src/pyobject.rs
Outdated
@@ -512,21 +512,30 @@ impl fmt::Debug for PyObject { | |||
#[derive(Debug, Default, Clone)] | |||
pub struct PyFuncArgs { | |||
pub args: Vec<PyObjectRef>, | |||
// TODO: add kwargs here | |||
pub kwargs: Option<Vec<(String, PyObjectRef)>>, |
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.
Finally 👍
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.
Btw, is option required? An empty vector means no kwargs?
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.
Nope, not required; just pushed up a fix.
No description provided.