Lesser changes and possible bugs encountered adding location information #1413 #1414
Conversation
in adding location information. The hope here is to reduce the size and effort for dealing with the location changes.
| summary_text = "match to any single expression" | ||
|
|
||
| def match(self, expression: Expression, pattern_context: dict): | ||
| def match(self, expression: BaseElement, pattern_context: dict): |
There was a problem hiding this comment.
Change to BaseElement reduces mypy complaints. It may very well be that Expression is more correct, but I don't know how to get mypy to agree with this. So here I switched rather than argue with mypy.
| """ | ||
| Load definitions from Builtin classes, autoload files and extension modules. | ||
| """ | ||
| from mathics.core.load_builtin import ( |
There was a problem hiding this comment.
I ran into circular dependencies from changes in #1413, and this fixed that.
However, moving this down also feels right in that there are the other imports here.
What this is telling us is that load_buildin_definitions() should be moved out of this module.
| # We have the character name ImaginaryI and ImaginaryJ, but we should | ||
| # have the *operator* name, "I". | ||
| special_symbols["\uF74F"] = special_symbols["\uF74E"] = "I" | ||
| special_symbols["\uf74f"] = special_symbols["\uf74e"] = "I" |
There was a problem hiding this comment.
This one is weird in that black or some formatter decided to do this.
Again, I'd rather switch than argue with whatever formatter decided this.
|
|
||
|
|
||
| def parse(definitions, feeder: LineFeeder) -> Any: | ||
| def parse(definitions, feeder: LineFeeder) -> Optional[BaseElement]: |
There was a problem hiding this comment.
Adding more precise type annotations for mypy.
| return (1, 1) | ||
|
|
||
| @property | ||
| def short_name(self) -> str: |
There was a problem hiding this comment.
Using short_name came up in working on TraceEvaluation. I am surprised we never needed short_name for patterns before.
| for rule in rules: | ||
| result = rule.apply(self, evaluation, fully=True) | ||
| if result is not None and not result.sameQ(self): | ||
| if result.is_literal: |
There was a problem hiding this comment.
This is probably just an oversight on our part. Stuff like this becomes apparent when you start looking at traces of expression evaluations.
| """ | ||
|
|
||
| pass | ||
| @property |
There was a problem hiding this comment.
I thiink @mmatera had mentioned that redfining True and False in MMA, if it is possible, will horribly break MMA.
I recall we had a discussion about whether the parent class SymbolConstant could be a literal. Right now it is not. However True and False I have a harder time seeing why it wouldn't be literal.
There was a problem hiding this comment.
Indeed, True and False, like List are some of the few symbols having the attribute Locked. I guess that this is because the evaluation relies on the fixed value of these symbols (but it is not especified anywhere)
| * the evaluation is a literal that evaluates to the same thing, | ||
| * evaluating a Symbol which the Symbol. | ||
| * Showing the return value of a ListExpression literal | ||
| * Evaluating Pattern[] which define a pattern. |
There was a problem hiding this comment.
This change is inspired by looking at TraceEvaluation. There is no benefit in watching an evaluation of _, n_, or n_Integer.
These are changes and slight bugs encounted in PR #1413
As a separate PR, this should reduce the size and effort for dealing with #1413, which I'll rebase after this is merged