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

[ysh] Implement SparseArray $[sp1 === sp2], append ... (sp), and $[bool(sp)] #2209

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Dec 31, 2024

Each commit corresponds to $[sp1 === sp2], append ... (sp), or $[bool(sp)].

@akinomyoga akinomyoga changed the base branch from master to soil-staging December 31, 2024 14:47
if len_lhs != len_rhs:
return False

for index in lhs.d.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove .keys() here, since it will make a copy

In Python dictionaries iterate over their keys by default (which is unlike Go, which may be confusing!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes! Thank you for checking these in detail! This is the same as #2167 (comment) and #2169. I created the initial version of the commits in this PR even before opening #2167 and #2169, but I forgot to update the commits after #2167 and #2169. I'll update it.

Copy link
Collaborator Author

@akinomyoga akinomyoga Dec 31, 2024

Choose a reason for hiding this comment

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

I updated it: 1121bfd..6726ba3

@andychu andychu merged commit b5e15f3 into soil-staging Dec 31, 2024
18 checks passed
@andychu
Copy link
Contributor

andychu commented Dec 31, 2024

Thanks!

@akinomyoga akinomyoga deleted the SparseArray-ysh branch December 31, 2024 18:31
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