Skip to content

Conversation

@rocky
Copy link
Member

@rocky rocky commented Oct 26, 2022

No description provided.

@rocky rocky marked this pull request as draft October 26, 2022 10:04
@rocky rocky force-pushed the add-form-to-output branch 2 times, most recently from 7d14b3b to bfbeb98 Compare October 26, 2022 12:42
@rocky rocky marked this pull request as ready for review October 26, 2022 17:33
In mathicsscript we find that PrintForms and OutputForms are the empty
set otherwise. Note mystery around why we cannot use "importlib"
@rocky rocky force-pushed the add-form-to-output branch from bfbeb98 to 371f365 Compare October 26, 2022 17:37
Copy link
Contributor

@mmatera mmatera left a 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")

Copy link
Contributor

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.

Copy link
Member Author

@rocky rocky Oct 26, 2022

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.

Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Member Author

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)

@rocky rocky merged commit 33b250c into master Oct 26, 2022
@rocky rocky deleted the add-form-to-output branch October 26, 2022 20:23
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.

3 participants