Updates to build with the latest SDK#179
Conversation
dennwc
left a comment
There was a problem hiding this comment.
Looks good, only few small comments:
| "Type": UASTType(uast.FunctionType{}, Fields{ | ||
| {Name: "Arguments", Op: Var("arguments")}, | ||
| {Name: "Returns", Optional: "ret_opt", Op: Cases("ret_case", | ||
| Is(nil), |
There was a problem hiding this comment.
Python functions always have an implicit return, so should be a single empty Argument with Init = None when there are no type annotations. There is a similar piece of code in JS).
There was a problem hiding this comment.
I implemented this and it worked beautifully... but then I noticed that this annotations only has a value IF the function has a Python 3.4+ type annotation BUT if it doesn't have it, it doesn't mean that the function returns None! Just that it's not annotated. So I think is better to left it with the current state (nil if there is no annotation, the annotation if there is one) because otherwise the UAST user could be confused into thinking that some functions return None while in fact they return other types.
Edit: will add a comment to clarify this.
There was a problem hiding this comment.
I haven't meant that the function returns None as type, only that if there is no return, all Python2 and Python3 functions returns None implicitly. So it's Returns.Argument.Init: None, same way as we do it in JS with undefined.
There was a problem hiding this comment.
Yes, but to know if the function returns None implicitly we should parse the function and find the return statement in this case.
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]> Remove some commented code, add some comments
Co-Authored-By: juanjux <[email protected]>
Co-Authored-By: juanjux <[email protected]>
- Fixes from review. - Add comments to the FunctionGroup. - Add comments to boxes attributes. Signed-off-by: Juanjo Alvarez <[email protected]>
… that the function returns None Signed-off-by: Juanjo Alvarez <[email protected]>
|
Ready for another review round! |
bzz
left a comment
There was a problem hiding this comment.
LGTM
but the arguments/ask for adding more information to commits with such changes in drivers (example of code + link to language speck + XPath to highlight the change) from elsewhere still holds.
Co-Authored-By: juanjux <[email protected]>
This fix some of the fields detected by the new check and explicitly ignores others, with comments and link to #178 in those cases.