Skip to content

[ysh] implement List remove() & List insert() - #2247

Merged
andychu merged 12 commits into
soil-stagingfrom
list-methods
Feb 16, 2025
Merged

[ysh] implement List remove() & List insert()#2247
andychu merged 12 commits into
soil-stagingfrom
list-methods

Conversation

@wlcx

@wlcx wlcx commented Feb 12, 2025

Copy link
Copy Markdown
Collaborator

Add missing List->remove() and List->insert() methods.

Please do suggest improvements to tests. I'm also assuming that mirroring the behaviour of python's remove and insert methods is alright.

@wlcx wlcx changed the title [ysh] implement List remove() [ysh] implement List remove() & List insert() Feb 13, 2025

@andychu andychu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks excellent, thank you!

Comment thread builtin/method_list.py Outdated
Comment thread builtin/method_list.py Outdated
Comment thread doc/ref/chap-type-method.md Outdated

Insert an element into the list at the given.
Negative indices index backwards from the back of the list. Indices greater than
than the number of elements in the list insert at the back of the list.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for documenting these edge cases!

Comment thread spec/ysh-list.test.sh Outdated
@andychu

andychu commented Feb 15, 2025

Copy link
Copy Markdown
Contributor

Ah for the BigInt issue, we can use mops.BigTruncate() (since I think it's OK for List to be limited to 2^32 entries)

We distinguish between integers in the type system:

  • Python int is C++ int, which is 32 bits
  • Python BigInt is C++ BigInt, which is currently 64 bits

I will check out the branch and make it compile

Thanks for adjusting the algorithm!

http://op.oilshell.org/uuu/github-jobs/9091/

@wlcx

wlcx commented Feb 15, 2025

Copy link
Copy Markdown
Collaborator Author

@andychu thanks! I wonder if it would be easy to catch that in the dev build...

I should have tried to do a non dev build locally I guess.

Let me know if the algo looks alright, not used to implementing these sort of things myself!

@andychu

andychu commented Feb 15, 2025

Copy link
Copy Markdown
Contributor

Usually I "lean" on the CI -- it catches a lot of problems

It is OK to just hack and see what breaks!

I made a few changes and now it passes

http://op.oilshell.org/uuu/github-jobs/9092/


I also am wondering about the negative overflow case .... I wrote a test to show that Python always inserts it at the beginning

e.g. if the list is length 10, then inserting at -11 -12 -13 -14 always puts it at the beginning (which is perhaps not intuitive)

Let me compare with what we're doing

@wlcx

wlcx commented Feb 15, 2025

Copy link
Copy Markdown
Collaborator Author

Does my last test case not catch that?

@andychu

andychu commented Feb 16, 2025

Copy link
Copy Markdown
Contributor

Ah OK I think the negative overflow may have been right -- it was hard for me to tell by reading it

Although the negative index case seems off by one, at least compared with Python:

If you insert at -1, Python does this:

>>> a=[1,2,3]
>>> a.insert(-1, 99)
>>> a
[1, 2, 99, 3]

The code was making it like [1,2,3,99] instead.

I don't think I've ever use this negative behavior, but I tend to follow Python and JS unless there's a reason not to

I fixed that, so I think it's right now


Thanks for the help! We can use help on other missing methods, as you saw in doc/ref/toc-ysh

And keep us updated on Zulip on the YSH-ing :-)

@andychu

andychu commented Feb 16, 2025

Copy link
Copy Markdown
Contributor

And BTW I don't expect contributors to fix the C++ stuff, because it's essentially a custom language

(although we document it here if interested -- https://oils.pub/release/0.26.0/doc/oils-repo/mycpp/README.html . Actually mycpp is no longer that hacky, so I should remove that disclaimer ... )

Doing the Python implementation is always the first step, and that's very helpful!

After awhile you get used to it, it's kinda fun to write typed Python, and then have it "magically" be 2x-50x faster

@andychu
andychu changed the base branch from master to soil-staging February 16, 2025 02:04
@andychu
andychu merged commit d1da625 into soil-staging Feb 16, 2025
@wlcx

wlcx commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator Author

Nice! Thanks for the info and for an excellent new contributor experience :)

My bad on the -ve indexing, I think I just assumed without checking pythons behaviour hahah

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