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

Move code out of definitions #631

Closed
wants to merge 3 commits into from
Closed

Conversation

rocky
Copy link
Member

@rocky rocky commented Nov 20, 2022

This moves builtin-creation out of mathics.builtin and core.definitions and into a new module.

I thing this is the last step before we actually move builtin creation of the Definitions initialization and into some one-time code.

@rocky rocky marked this pull request as draft November 20, 2022 15:20
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

Copy link
Contributor

@TiagoCavalcante TiagoCavalcante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I'm just wondering now why MakeBoxes has to contribute first

@rocky
Copy link
Member Author

rocky commented Nov 20, 2022

LGTM - I'm just wondering now why MakeBoxes has to contribute first

It is not now - it was changed to be that way recently. And I wondered too why MakeBoxes has to contribute first. @mmatera had one of his long explanations, given the current code in master and the involved process it follows, why this makes things work.

You can probably find that in a recent PR that about "fixing" Makeboxes.

I wonder, though, if there isn't a simpler organization and way to do things that does not require this kind of special logic. We have seen this kind of thing over and over.

But for now, I am struggling just to pull stuff out of mathics.builtin and then the Definitions object. And we've seen this kind of behavior too - we can't begin to do what we want because the logic of code is a bit involved, not well written or documented.

The wrongness of rereading module definitions over and over goes back to the earliest days V 0.5. But since then instead of addressing what I think is the root cause building definitions once, a complicated custom module cache code was added as a workaround. (Okay - caching allows the not-needed feature that you can run mathics, change a Python module, and possibly do something that will cause the module to reload and then see that new code.)

Then pymathics was added, and then this MakeBoxes fix was added (or is in the process of being added?)

Fixing the V 0.5 version is much easier than fixing the code as it is now. This is what happens when you build on a features on a foundation that isn't solid to begin with.

@mmatera
Copy link
Contributor

mmatera commented Nov 20, 2022

It is because other symbols can add rules to the MakeBoxes definition. Hopefully, we are going to get ride it this behavior

@rocky rocky force-pushed the create-builtin-system_init branch 2 times, most recently from 9c21740 to da8580c Compare November 21, 2022 19:29
This moves builtin-creation out of mathics.builtin and core.definitions
and into a new module.

I thing this is the last step before we actually move builtin creation
of the Definitions initialization and into some one-time code.
and add run pytest setting MATHICS_CHARACTER_ENCODING
rocky added a commit that referenced this pull request Dec 7, 2022
These changes have been split off from #631. That other PR will have to
be rebased after this and #651 go in.
rocky added a commit that referenced this pull request Dec 7, 2022
Miscelleneous lint found in files of #631
@rocky rocky closed this Oct 20, 2024
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