-
-
Notifications
You must be signed in to change notification settings - Fork 65
Move import forms *before* use of PrintForms #587
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
7d14b3b to
bfbeb98
Compare
In mathicsscript we find that PrintForms and OutputForms are the empty set otherwise. Note mystery around why we cannot use "importlib"
bfbeb98 to
371f365
Compare
mmatera
left a comment
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
| # imported. This module populates PrintForms and OutputForms. | ||
| if not PrintForms: | ||
| importlib.import_module("mathics.format") | ||
|
|
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 not directly related to PrintForm. This module contains the routines that render strings from BoxExpressions.
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.
By PrintForms (note final "s"), I mean the module variable PrintForms (set in self.print_forms) or exposed variable $PrintForms. If you try the branch in mathicsscript that has this name add-to-form and run it against master you will find that the set variables in this module PrintForms and OutputForms are the empty set.
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, I was talking about that variable, sorry.
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.
PrintForms must be populated when mathics.builtin.forms load each subclass of FormsBaseClass, but it should import mathics.core.definitions to do that. I can investigate it latter
|
|
||
| # importing format loads populates Symbols to PrintForms and OutputForms | ||
| # Rocky: this smells of something not quite right in terms of modularity. | ||
| import mathics.format # noqa |
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.
Indeed, it is a side effect of lack of modularity. The problem is that mathics.format requires mathics.builtin.box, and it depends on mathics.builtin.
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.
Interesting. In which line this happens?
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.
It is not an error, it is just that the output won't get tagged (which is the point of the change) since result.form will be None because nothing in output_forms:
+ if head in output_forms:
+ form = self.definitions.shorten_name(head.name)
No description provided.