-
Notifications
You must be signed in to change notification settings - Fork 227
starlark: Add Module type #595
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
starlark/value.go
Outdated
// variables so far defined in the function's module. | ||
func (fn *Function) Globals() StringDict { return fn.module.makeGlobalDict() } | ||
|
||
// Predeclared returns a new StringDict containing the | ||
// predeclared names in the function's module. | ||
func (fn *Function) Predeclared() StringDict { |
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 wonder if it would make more sense to expose the Module type in the API (keeping its fields private), and to add the Predeclared method onto Module? That way it is clear that fn.Module().Predeclared()
accesses a value that is shared among all functions of the same module (even if there's today nothing else you can do with the module--though in future there may be).
Also, unlike Globals, Predeclared should just return a reference to the StringDict, not a copy, since the user provided us with it in the first place.
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.
Should Globals live on this Module type too then?
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.
Yes, that seems appropriate, though of course we'll have to keep the existing method on Function as a wrapper.
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.
Done, PTAL.
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.
Test looks good; thanks.
fixed all the comments |
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.
LGTM, thanks again.
This PR adds a new Module type and surrounding API:
The existing
(*Program).Init
function is reimplemented in terms ofInitModule
. Similarly,fn.Globals()
is now the same asfn.Module().Globals()
.Specifically, this adds a way to get the predeclared dictionary for a given Function, which makes it easier to write a debugger.
Fixes #594