-
-
Notifications
You must be signed in to change notification settings - Fork 61
Fix set 3rd round #619
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
Closed
Closed
Fix set 3rd round #619
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f4eb4ce
Add SetAttributes length check and ...
rocky 069deea
Merge pull request #615 from Mathics3/SetAttribute-arg-check
rocky c1b5fa3
fix get_sort_key second argument and BoxExpression.sameQ. With this, …
mmatera 475cb9d
adding test
mmatera 1f87088
simplifying assignment. Now all the tests pass.
mmatera 784e723
simplify and improve assign internals
mmatera c216a93
comment on process_rhs_conditions
mmatera 2ffdf02
Correct LHS evaluation in SetDelayed assignment (#603)
mmatera ed3b5bb
merge
mmatera d69e338
merge
mmatera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked this up in the code. You are correct - this method named
applyis 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.