Split mathics.core.assignment into mathics.core.assignment and mathics.eval.assignment #1264
Conversation
|
Thanks for undertaking this. (I think somewhere I see Builtin misspelled - running through |
mathics/eval/assignment.py
Outdated
| class _SetOperator: | ||
| """ | ||
|
|
||
| This is the base class for assignment Builtin operators. |
There was a problem hiding this comment.
Stuff in eval probably should not have classes with objects. The focus of eval is an interpreter that knows nothing about OO. You should be able to write this code in non-OO language like C, golang, or assembly.
It is okay to pass OO objects or "self" pointers though. For these, there are object dereferences which is okay to do. Dereferencing is different from implicit OO method lookup which goes on inside classes.
In sum, this class probably belongs in either mathics.core (similar to Atom classes) or mathics.builtin.
There was a problem hiding this comment.
Grepping mathics.eval right now I see:
$ PAGER=cat git grep -n "class "
distance/clusters.py:8:class IllegalDataPoint(Exception):
distance/clusters.py:12:class IllegalDistance(Exception):
files_io/read.py:42:class MathicsOpen(Stream):
patterns.py:11:class _StopGeneratorMatchQ(StopGenerator):
patterns.py:15:class Matcher:
pymathics.py:21:class PyMathicsLoadException(Exception):
rules.py:149:class Dispatch(Atom):
test.py:12:class _StopGeneratorBaseElementIsFree(StopGenerator):Exceptions and Generators are weird kinds of classes, so there may be some wiggle room here. But Dispatch(Atom) and Matcher should probably be moved.
There was a problem hiding this comment.
The problem I found is that putting this class in another place produces circular dependencies among modules. I am trying to figure out how to avoid it.
There was a problem hiding this comment.
Easy: just get ride of the class...
mathics/core/assignment.py
Outdated
| @@ -223,32 +228,33 @@ def rejected_because_protected( | |||
|
|
|||
| def unroll_conditions(lhs: BaseElement) -> Tuple[BaseElement, Optional[Expression]]: | |||
| """ | |||
| If lhs is a nested `Condition` expression, | |||
| If `element` is a nested `Condition` expression, | |||
There was a problem hiding this comment.
Does "element" refer to "lhs" on line 229?
mathics/core/assignment.py
Outdated
| @@ -5,12 +5,12 @@ | |||
| Support for Set and SetDelayed, and other assignment-like builtins | |||
| """ | |||
|
|
|||
| from typing import Callable, List, Optional, Tuple | |||
| from typing import Callable, List, Optional, Tuple, cast | |||
There was a problem hiding this comment.
Using "cast" is a bit of a cop-out. It just postpones fixing things properly down the line. So if we can fix the problem without casting that would be nice.
But sure, I understand we have a big mess to deal with.
There was a problem hiding this comment.
Yep, I am trying to figure out how to deal with functions that return Optional[...]. In any case, this will need a couple of rounds to get ready...
There was a problem hiding this comment.
I have long had this annoying feeling that our classes, like BaseElement, have been a little bit wrong.
It has too many functions that are not universally applicable.
But I have deferred undertaking addressing this until I understand the code better and what to do.
And in the other direction, it might be the case that everything should have a "head" and "elements", and perhaps "value". And if we did that there would be fewer testing of types and simpler code.
There was a problem hiding this comment.
I think that only Expression and their subclasses should have head and elements properties.
On the other hand, in cases like this, where you want to use polymorphism, maybe using cast is the best solution.
In this module, I used it in two cases: one is the conversion from BaseElement to Expression. The other is to use a definition obtained from Definitions.get_definition.
In the second case, the problem is that get_definition returns None when the symbol does not have a definition. There would be two alternatives:
- create an empty definition: This is costly and useless. We want to know as early as possible that a symbol does not have a definition, to avoid asking questions to it. Create an empty definition would introduce a lot of unnecesary overhead.
- raise an exception: maybe this would be a more pythonic solution, just that I do not finish to get used to think of exceptions as something which is part of the standar logic of a code.
There was a problem hiding this comment.
On the other hand, in cases like this, where you want to use polymorphism, maybe using cast is the best solution.
I speak from lots of experience in other programming languages: do not go down this path.
There was a problem hiding this comment.
OK. In any case, using cast is a good way to detect where the code get's fishy.
There was a problem hiding this comment.
OK, now I was able to get ride of all casts
There was a problem hiding this comment.
Thank you. I greatly appreciate the attention to improving code quality.
|
I think this is a good moment to consider merging this before continuing. |
Give me a couple of days to look at. I have been meaning to see what's up with stopit first. |
OK. There is no hurry. On the other hand, this is just juggling a little bit with some names and distribution of the code, and completing the documentation and type annotations. |
mmatera
changed the base branch from
master
to
removing_optional_in_get_definitions
| rhs: BaseElement, | ||
| evaluation: Evaluation, | ||
| ) -> Optional[BaseElement]: | ||
| "tag_ /: lhs_ = rhs_" |
There was a problem hiding this comment.
Actually, f_ is correct here since this is how the documentation describes the parameter. I will be putting in a PR for this and other changes.
mathics/core/assignment.py
Outdated
| elif name == "System`HoldPattern": | ||
| lhs = lhs_elements[0] | ||
| return lhs, rhs | ||
| # In the folliwing routines |
|
LGTM |
rocky
merged commit c2e91ab
into
removing_optional_in_get_definitions
And adding docstrings and type annotations.