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
Builtin functions refactoring #169
Conversation
62cdded
to
484f2c6
Compare
2c9dae2
to
008efe0
Compare
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 a huge fan of the "random" distinction between @builtin
and @builtin_names
, and with the naming of the functions. Could they be made more consistent?
I'd probably go with always having a "builtin_" prefix to the function name and always using @builtin_names
@@ -562,8 +562,7 @@ def industry_town_count(name, args, pos, info): | |||
return (args[0], extra_params) | |||
|
|||
def industry_cargotype(name, args, pos, info): | |||
from nml.expression.functioncall import builtin_cargotype | |||
return (builtin_cargotype(name, args, pos), []) | |||
return (expression.functioncall.builtin_resolve_typelabel(name, args, pos), []) |
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.
builtin_resolve_typelabel would not need that "default to cargo" trick, if there was a 4th optional paramater for the function name, like
builtin_resolve_typelabel("cargo", args, pos, username=name)
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 really don't like this idea. The existing name
parameter already has that meaning; passing in a fake value to manipulate the behaviour and then adding a new parameter for the real value seems like a far bigger hack. The default is going to exist somewhere, why not here?
(I updated the comment a bit)
008efe0
to
77dfb51
Compare
77dfb51
to
ce3fac6
Compare
I changed |
Yay, more random cleanups.
Slightly RFC on the decorator bit; if this style makes sense there are probably more things to apply it to. Personally I think anything is better than these big tables.