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

Builtin functions refactoring #169

Merged
merged 2 commits into from Dec 5, 2020
Merged

Conversation

FLHerne
Copy link
Contributor

@FLHerne FLHerne commented Nov 9, 2020

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.

@FLHerne FLHerne force-pushed the flh-builtins-cleanup branch 3 times, most recently from 2c9dae2 to 008efe0 Compare November 9, 2020 13:53
Copy link
Member

@LordAro LordAro left a 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), [])
Copy link
Member

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)

Copy link
Contributor Author

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)

@FLHerne
Copy link
Contributor Author

FLHerne commented Dec 5, 2020

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

I changed @builtin to require and strip off the "builtin_" prefix, which removes most of the inconsistency. Perhaps I'm overthinking this, but it should reduce the chance of copy-paste errors if someone adds a new function.

@FLHerne FLHerne merged commit 827823a into OpenTTD:master Dec 5, 2020
@FLHerne FLHerne deleted the flh-builtins-cleanup branch April 2, 2021 22:17
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.

None yet

4 participants