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 FLHerne:flh-builtins-cleanup branch from 62cdded to 484f2c6 Nov 9, 2020
@FLHerne FLHerne force-pushed the FLHerne:flh-builtins-cleanup branch 3 times, most recently from 2c9dae2 to 008efe0 Nov 9, 2020
Copy link
Member

@LordAro LordAro left a comment

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), [])

This comment has been minimized.

@frosch123

frosch123 Dec 4, 2020
Member

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)

This comment has been minimized.

@FLHerne

FLHerne Dec 5, 2020
Author Contributor

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 FLHerne force-pushed the FLHerne:flh-builtins-cleanup branch from 008efe0 to 77dfb51 Dec 5, 2020
@FLHerne FLHerne force-pushed the FLHerne:flh-builtins-cleanup branch from 77dfb51 to ce3fac6 Dec 5, 2020
@FLHerne
Copy link
Contributor Author

@FLHerne 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
21 checks passed
21 checks passed
Commit checker
Details
Python 3.5 on ubuntu-latest
Details
Security and Quality
Details
Python 3.6 on ubuntu-latest
Details
Python 3.7 on ubuntu-latest
Details
Python 3.8 on ubuntu-latest
Details
Python pypy3 on ubuntu-latest
Details
Python 3.5 on macOS-latest
Details
Python 3.6 on macOS-latest
Details
Python 3.7 on macOS-latest
Details
Python 3.8 on macOS-latest
Details
Python 3.5 on windows-2016
Details
Python 3.6 on windows-2016
Details
Python 3.7 on windows-2016
Details
Python 3.8 on windows-2016
Details
Python 3.x on ubuntu-latest
Details
Python 3.x on macOS-latest
Details
Python 3.x on windows-2016
Details
Flake8
Details
Black
Details
CodeQL 13 notes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants