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

[mojo-stdlib] Add list.insert(index, value) API to stdlib #2148

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

whym1here
Copy link

@whym1here whym1here commented Apr 2, 2024

This pr serves to add list.insert(index, value) API to the stdlib as requested by issue #2134.

This API should handle the same functionality as the python list.insert and adds tests for the same.

@whym1here whym1here requested a review from a team as a code owner April 2, 2024 14:51
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

Looks good! We'll need to rebase this PR/resolve conflicts and then add a test for the negative index case.

Currently, the PR is showing many commits from April 4th that already exist in the nightly branch. If you use git rebase -i upstream/nightly where upstream is the corresponding git remote:

git remote -v
upstream        [email protected]:modularml/mojo.git (fetch)
upstream        [email protected]:modularml/mojo.git (push)

then you'll be able to see those commits not show up as duplicates in your PR. Please let me know if you run into any issues and I'll be happy to help.

stdlib/src/collections/list.mojo Show resolved Hide resolved
stdlib/test/collections/test_list.mojo Show resolved Hide resolved
@whym1here whym1here force-pushed the mojo-list-insert branch 2 times, most recently from c97ff8c to 9e29bdf Compare April 6, 2024 18:29
@whym1here whym1here requested a review from JoeLoser April 6, 2024 19:07
@JoeLoser
Copy link
Collaborator

JoeLoser commented Apr 8, 2024

Looks like CI is failing due to formatting. Do you mind running mojo format -l 80 stdlib/src/collections/list.mojo stdlib/test/collections/test_list.mojo to fix the formatting issues? Thanks!

Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

LGTM - just need to fix the formatting before we merge this. Thank you for your contribution! 🚀

assert_equal(vec[1], 2)
assert_equal(vec[2], 3)

vec.clear()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion Instead of manually running clear() between the different parts of this function, it would read better to just use different variables, such as v1 = List[Int](), v2 = List[Int](), and so on. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah seems like a good idea

@JoeLoser JoeLoser merged commit 98e062d into modular:nightly Apr 11, 2024
5 checks passed
@whym1here whym1here deleted the mojo-list-insert branch April 11, 2024 18:36
arvindavoudi pushed a commit to arvindavoudi/mojo that referenced this pull request Apr 13, 2024
Add `List.insert(index, value)` API as requested in modular#2134.

This API should handle the same functionality as the Python `list.insert` and adds tests for the same.

Fixes modular#2134

Signed-off-by: Dhaval Kumar <[email protected]>
patrickdoc added a commit to patrickdoc/mojo that referenced this pull request Apr 24, 2024
…odular#2148) (#38047)

Add `List.insert(index, value)` API as requested in
[Internal Link]

This API should handle the same functionality as the Python
`list.insert` and adds tests for the same.

Fixes [Internal Link]

Signed-off-by: Dhaval Kumar <[email protected]>

mojo-orig-commit: 98e062d

modular-orig-commit: 5128d9ad94ee7acb9b876d3738e7ce47405afdab
jayzhan211 pushed a commit to jayzhan211/mojo that referenced this pull request Apr 27, 2024
…odular#2148) (#38047)

Add `List.insert(index, value)` API as requested in
[Internal Link]

This API should handle the same functionality as the Python
`list.insert` and adds tests for the same.

Fixes [Internal Link]

Signed-off-by: Dhaval Kumar <[email protected]>

mojo-orig-commit: 98e062d

modular-orig-commit: 5128d9ad94ee7acb9b876d3738e7ce47405afdab
patrickdoc added a commit to patrickdoc/mojo that referenced this pull request May 2, 2024
…odular#2148) (#38047)

Add `List.insert(index, value)` API as requested in
modular#2134.

This API should handle the same functionality as the Python
`list.insert` and adds tests for the same.

Fixes modular#2134

Signed-off-by: Dhaval Kumar <[email protected]>

mojo-orig-commit: 98e062d
MODULAR_ORIG_COMMIT_REV_ID: 5128d9ad94ee7acb9b876d3738e7ce47405afdab
patrickdoc added a commit that referenced this pull request May 2, 2024
…2148) (#38047)

Add `List.insert(index, value)` API as requested in
#2134.

This API should handle the same functionality as the Python
`list.insert` and adds tests for the same.

Fixes #2134

Signed-off-by: Dhaval Kumar <[email protected]>

mojo-orig-commit: 98e062d
MODULAR_ORIG_COMMIT_REV_ID: 5128d9ad94ee7acb9b876d3738e7ce47405afdab
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.

4 participants