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

Clean up oversights and leftovers for array refactoring #2223

Merged
merged 7 commits into from
Jan 4, 2025

Conversation

akinomyoga
Copy link
Collaborator

No description provided.

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

I think changing the user-facing strings may not be worth it, because we have to change it back later

raise error.TypeErr(val, 'Expected BashArray or BashAssoc',
blame_loc)
raise error.TypeErr(
val, 'Expected BashArray, SparseArray, or BashAssoc',
Copy link
Contributor

Choose a reason for hiding this comment

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

We are eventually going to remove change SparseArray -> BashArray, so this change will have to be undone

Since it is a user-facing string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if it would be finally switched, I wanted to carry out the switching in a single consistent commit.

I dropped commit 8e3f4b1. I've already modified the other error messages, but they are not reverted within this PR.

builtin/pure_ysh.py Outdated Show resolved Hide resolved
@akinomyoga akinomyoga force-pushed the SparseArray-leftover branch from ed5b029 to 9137175 Compare January 4, 2025 21:56
@andychu andychu merged commit 7b4f24c into soil-staging Jan 4, 2025
18 checks passed
@andychu
Copy link
Contributor

andychu commented Jan 4, 2025

Great, thanks

@akinomyoga akinomyoga deleted the SparseArray-leftover branch January 5, 2025 01:12
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