-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Two bugs found via another PR #636
Conversation
rules.py __setstate__() `mathics.builtin.builtins` was renamed to `mathics.builtin._builtins` I guess on the assumption it was private. It turns out it wasn't. And we were using the older (more correct) name "builtins" instead of "_builtins". Use "_builtins" to make this work. `__setstate__` apparently isn't used right now, but would be if we used `deepcopy()`. Note that in another PR, this variable is renamed `system_builtins_dict`. rest: Change imports of Builtin from `mathics.builtin` to mathics.builtin.base. `mathics.builtin only accidentially imports `Builtin`.
e8c5c50
to
6844555
Compare
(After this goes in there will be a small merge conflict with #635 but that should be easy to fix) |
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
|
||
self.function = getattr(builtins[cls], name) | ||
self.function = getattr(_builtins[class_name], 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 Just wonder, is it OK to keep the name _builtins
for something that is not private to the module?
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.
This is a temporary change. #635 or a stripped down version of that has the more complete fix.
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.
In that case, let's merge and iterate.
rules.py __setstate__()
:mathics.builtin.builtins
was renamed tomathics.builtin._builtins
I guess on the assumption it was private.
It turns out it wasn't. And we were using the older (more correct) name "builtins" instead of "_builtins". Use "_builtins" to make this work.
__setstate__
apparently isn't used right now, but would be if we useddeepcopy()
. Note that in #635, this variable is renamedsystem_builtins_dict
.rest:
Change imports of Builtin from
mathics.builtin
tomathics.builtin.base
.mathics.builtin
only accidentially importsBuiltin
.