Fix set 3rd round #619
Fix set 3rd round #619
Conversation
* More apply() -> eval() cutovers in the changed files * Some <dt> indentation regularization. * DRY redundant "argr" message definitions
Add SetAttributes length check and ...
…SyntaxAnnotation.m does not kill the interpreter
| while the second, | ||
| ``p[-4]/; -4>0`` | ||
|
|
||
| This should be handled in the apply method of the BaseRule class. |
There was a problem hiding this comment.
This was going to be my point as well. Too often it feels that when we try to run to run some WL code and that fails, the immediate solution is well, let's just add a bunch of flags and tests on symbols in Python code address the specific case we see.
This is done instead of seeing the problem as a manifestation of some larger structural problem, or feature lacking in the way particular built-in functions should work.
I would prefer to see fixing the root cause issues in favor of the workaround coding.
(You probably mean eval method, but okay.)
There was a problem hiding this comment.
This was going to be my point as well. Too often it feels that when we try to run to run some WL code and that fails, the immediate solution is well, let's just add a bunch of flags and tests on symbols in Python code address the specific case we see.
No, the idea is to identify these issues, and then try to understand how it works and how it should work, and then fix it.
This is done instead of seeing the problem as a manifestation of some larger structural problem, or feature lacking in the way particular built-in functions should work.
A manifestation of a large structural problem is the performance issue, and the fact that we can not load wl modules. All of this is about to narrow the different issues and try to determine what should be the pattern.
I would prefer to see fixing the root cause issues in favor of the workaround coding.
The workaround was there for a while. What I did here is to determine why this workaround was needed to reproduce the WMA behavior. As the picture gets clear, we can continue removing these issues, like the one with oneidentity. What I am trying to avoid is to pretend to fix many things in the same PR.
(You probably mean eval method, but okay.)
I said apply, because is the current name of the method. Then, if you think that is more clear/ accurate to say "evaluate a replacement rule over an expression" over "apply a replacement rule over an expression" let's change the name of the method.
There was a problem hiding this comment.
The workaround was there for a while. What I did here is to determine why this workaround was needed to reproduce the WMA behavior. As the picture gets clear, we can continue removing these issues, like the one with oneidentity. What I am trying to avoid is to pretend to fix many things in the same PR.
So what I am thinking is let's focus on removing the workarounds that we can fix rather than changing workarounds.
I know of some on the horizon but that could be fixed now, like #597 and the box precedence operator. Stuff like this keeps rolling in though which from my standpoint isn't helping as much as it is delaying getting more root issues fixed.
There was a problem hiding this comment.
I said apply, because is the current name of the method. Then, if you think that is more clear/ accurate to say "evaluate a replacement rule over an expression" over "apply a replacement rule over an expression" let's change the name of the method.
I just looked this up in the code. You are correct - this method named apply is doing application, not evaluation. And this reinforces why it would be better to have terminology cleaned up - I often do not know if a term is used in the right sense or the wrong sense and conceptually they mean different things so I get more confused.
Ok - it would be helpful create detailed and narrowed issues for these. |
* improving clarity in Builtin.contribute * adding comments. adding tests * fix test for SetDelayed. Move special case for Set with LHS a list away from the conditional. * more on modularize assignment. assign_elementary->assign * fixing set_eval * adding tests for OneIdentity * fix OneIdentity * remove comment * fix pytest * fix a bug that makes that URLSave always fails * fix WriteString standard output * catch not known attributes in ClearAttributes and SetAttributes * Update 'attributes' for current standards `mathics.builtin.attributes`: * apply -> eval * Add WMA links add * Class names ordered alphabetically * Hard breaks in docstring removed * Some incorrect references to "leaves" changed to "attributes" `mathics.core.attributes`: * Add short comments above flag value * adding comments. adding tests * fix test for SetDelayed. Move special case for Set with LHS a list away from the conditional. * more on modularize assignment. assign_elementary->assign * Handle optional with a first element that is not a Pattern[] * Update examples in OneIdentity * More pervasive use of Symbols symbols.py: system_symbols() -> symbol_set(); "systems_symbols" name is too close Symbol( System` to module systemsymbols. Also, we now require symbols as parameters), not strings. systemsymbols.py: more system symbols * Pattern_create -> Pattern.create It appears this was originally Pattern.create. I suspect due to bad modularity and a lack of understandig Python that an import could be added inside the routine, this static method got moved outside of the class. Later on, the modularity was fixed, but the hack persisted. These kinds of code smells side effects of poor communication. * Add function signature; straighten import issue * Put test_rules_patterns tests where they belong * Add note to add skipped example as a doctest ... When it gets fixed. * Changes suggested in PR review Move core-like assignment interals out of builtins and into core. (We may want to split up core more in the future though) Better sort long list of assignment methods alphabetically Some small RstT tagging in a docstring Remove nonexistent word "evaluable" Add yet another type annotation to a signature Make test assert failure messages unique * Move ASSIGNMENT_FUNCTION_MAP into core Co-authored-by: R. Bernstein <rocky@users.noreply.github.com> Co-authored-by: rocky <rb@dustyfeet.com>
|
Superseded by #625 |
This completes the fixes needed to pass all the failing tests added in the first round.