Skip to content
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

Changes suggested in PR review #622

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Conversation

rocky
Copy link
Member

@rocky rocky commented Nov 15, 2022

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 RsT tagging in a docstring

Remove nonexistent word "evaluable"

Add yet another type annotation to a signature

Make test assert failure messages unique

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
@rocky rocky requested a review from mmatera November 15, 2022 03:24
# Below, we key on a string, but Symbol is more correct.
#

special_cases = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this dictionary could be moved also to marhics.core.assignment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Will do next.

Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The only problem with this is that I am going to have to rework the other three PR. In any case, ranch one just comprised small changes over this.

@rocky
Copy link
Member Author

rocky commented Nov 15, 2022

LGTM. The only problem with this is that I am going to have to rework the other three PR. In any case, ranch one just comprised small changes over this.

Please let's hold off on the others for now. It is not my intention to make additional work and I don't like to see this happen.

Make detailed issues describing the problem.

Something we keep running into is that basically the code doesn't follow more modern standards of having signatures, ordering imports. (There is a python module called isort that will do this). Naming eval methods with eval (when indeed that is what it is), adding WMA links and so on.

So maybe before modifying a module we should make a pass over that first.

@rocky rocky merged commit 73a369b into fix_set_eval Nov 15, 2022
@rocky rocky deleted the improve-SetDelayed-LHS-assignment branch November 15, 2022 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants