Skip to content

Symbol init cleanup 2 #691

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Symbol init cleanup 2 #691

wants to merge 12 commits into from

Conversation

@rocky
Copy link
Member

@rocky rocky commented Dec 27, 2022 •
edited
Loading

Thanks @mmatera to getting this to the point where I can see the light at the end of the tunnel.

@rocky rocky force-pushed the Symbol-init-cleanup_2 branch 5 times, most recently from ae421de to e3c545c Compare December 27, 2022 01:58
@rocky rocky mentioned this pull request Jan 3, 2023
Merged
rocky and others added 11 commits January 7, 2023 08:22
@rocky
WIP - separate PredefinedSymbol from Symbol ..
35f6d61 
PredefinedSymbol acts more like a numeric constant.
@rocky
Clarify Symbol as per mmatera
4d49f6b 
Update builtin/atomic/symbols.py according to current standards.
@mmatera @rocky
fix MakeBoxes[Infix[...]]
21a81ad
@mmatera @rocky
MakeBoxes[Infix[...],form]-> Infix
e8ca60a
@mmatera @rocky
alternative implementation
a417f00
@mmatera @rocky
moving do_eval_infix to be a non-member function in the same module.
889e9fc
@mmatera @rocky
adding comment
94f2bb0
@rocky
Move towards segregating box from eval
11a7eec 
* Some long lines split
* some linting
* correct a wrong symbol
@rocky
Adjust builtins_precedence and doc parenthesize
0d4ce19 
We should be using Symbols insead of string where possible.
builtins_precedence is really used in eval/makeboxes so it is defined
there.

It is just initialized in `mathics.buitins`.
@rocky
PredefinedSymbol -> SymbolConstant...
a43fd5e 
and try to clearify what is up here.
@rocky
tweak docstring, more type annotations
45c12c4
@rocky rocky force-pushed the Symbol-init-cleanup_2 branch from 2fde486 to 632b83a Compare January 7, 2023 14:04
@rocky
Copy link
Member Author

rocky commented Jan 7, 2023

@mmatera I have rebased this with the current master and made small changes to get this working.

Please look over all of the boxing changes here and advise if this is relevant, correct, and should go in.

@rocky rocky force-pushed the Symbol-init-cleanup_2 branch from 632b83a to c4c1e6d Compare January 7, 2023 14:12
@rocky
Copy link
Member Author

rocky commented Jan 7, 2023

@mmatera I have rebased this with the current master and made small changes to get this working.

Please look over all of the boxing changes here and advise if this is relevant, correct, and should go in.

Looking over myself, I am happy to put in a new PR with the boxing stuff removed until after release. I don't fully understand this, and the focus of this PR of this wasn't about boxing which somehow slipped in here as a bug fix for the Symbol work.

@mmatera
Copy link
Contributor

mmatera commented Jan 7, 2023

@rocky, as you prefer. The idea was to move out of mathics.builtin.makeboxes the implementation of parenthesize and make_boxes_infix, in the line of moving the evaluation logic outside the mathics.builtin module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TiagoCavalcante TiagoCavalcante Awaiting requested review from TiagoCavalcante

@mmatera mmatera Awaiting requested review from mmatera

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rocky @mmatera