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.

Sign up for GitHub

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

Jump to bottom

[stdlib] Rename `List.size` to `List._len` and refactor usage of the field to use the public API #3814

Closed

martinvuyk wants to merge 34 commits into modular:main from martinvuyk:make-list-size-private

Contributor

martinvuyk commented Nov 27, 2024 •

edited

Loading

Rename `List.size` to `List._len` and refactor usage of the field to use the public API.

Details:

Part of [BUG] The field storing the number of items in a List or Dict should be named "length", not "size" #3511
To prepare for any future implementation details than can change under the hood for List.

martinvuyk requested a review from a team as a code owner

November 27, 2024 00:39

Contributor Author

martinvuyk commented Dec 2, 2024

I'll leave this as draft until a decision is made of whether to deprecate accessing the fields directly. Since this implementation will have issues when trying to access the field with an immediate operation e.g. some_list.length += 1, which was solved only for some_list.size += 1 by making size the default getattr field name

martinvuyk marked this pull request as draft

December 2, 2024 17:48

JoeLoser approved these changes

View reviewed changes

Collaborator

JoeLoser commented Dec 4, 2024

Personal opinion: I'd like it if we could just deprecate letting people modify the length variable directly (removing setters and getters), and make them use the public API.

Let's do that and remove the setters/getters.

Can you please also add a changelog entry so people understand their compiler errors as a result of this change if they are accessing the length field directly of List?

JoeLoser added the waiting for response label

martinvuyk added 6 commits

December 4, 2024 10:14


          Rename List.size to List._len adding getattr and setattr for size

6d5e0fd

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          add length attribute overload

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix detail

b6e9e36

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix detail

720e043

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          try removing all uses of list.size

0f43949

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix uses of buf.size

2c7857d

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

martinvuyk force-pushed the make-list-size-private branch from 6e070d8 to 2c7857d Compare

December 4, 2024 13:52

martinvuyk added 4 commits

December 4, 2024 11:12


          fix pixi.toml diff

1cc618e

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix pixi.toml diff

4c4e1fe

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix details

6c178fb

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix details

8e88d34

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

martinvuyk changed the title ~~[stdlib] Rename List.size to List._len adding getattr and setattr for size~~ [stdlib] Rename List.size to List._len and refactor usage of the field to use the public API


          fix details

5bc5722

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

martinvuyk marked this pull request as ready for review

December 4, 2024 14:53

martinvuyk requested a review from JoeLoser

December 4, 2024 14:58

martinvuyk added 4 commits

December 9, 2024 22:21


          Merge remote-tracking branch 'upstream/nightly' into make-list-size-p…

e236c6f

…rivate


          fix details

abc6e26

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix details

554a0b6

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix details

a3bfe40

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

ConnorGray reviewed

View reviewed changes

stdlib/src/collections/list.mojo Outdated Show resolved Hide resolved

ConnorGray reviewed

View reviewed changes

stdlib/src/base64/_b64encode.mojo Outdated Show resolved Hide resolved

martinvuyk marked this pull request as draft

December 10, 2024 22:44

martinvuyk mentioned this pull request

[stdlib] Add List[Scalar[D]] append SIMD and Span[Scalar[D]] #3854

Closed

modularbot added a commit that referenced this pull request


          [External] [stdlib] Add List[Scalar[D]].extend() from SIMD and `S…

6eb0b89

…pan[Scalar[D]]` (#52584)

[External] [stdlib] Add `List[Scalar[D]].extend()` from `SIMD` and
`Span[Scalar[D]]`

Add `List[Scalar[D]].extend()` from `SIMD` and `Span[Scalar[D]]`

Split off from #3814. This is needed to enable efficient
appending of scalar value sequences to a `List` without having to resort
to `UnsafePointer` manually.

ORIGINAL_AUTHOR=martinvuyk
<110240700+martinvuyk@users.noreply.github.com>
PUBLIC_PR_LINK=#3854

Co-authored-by: martinvuyk <110240700+martinvuyk@users.noreply.github.com>
Co-authored-by: Connor Gray <code@connorgray.com>
Closes #3854
MODULAR_ORIG_COMMIT_REV_ID: 7d0c724497ba0671ae660f4de5758d6c4baad7bc

martinvuyk added 2 commits

February 2, 2025 09:55


          mojo format

d361412

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix assert message

a9a2609

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

Contributor Author

martinvuyk commented Feb 2, 2025

Hi @arthurevans thanks for the input, you're totally right that this needed some examples :)

martinvuyk added 3 commits

February 2, 2025 10:05


          fix details

498f4d3

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix more uses of string buffer constructor

58b81a5

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>


          fix more uses of string buffer constructor

87a1a53

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

modularbot added a commit that referenced this pull request


          [External] [stdlib] Add List[Scalar[D]].extend() from SIMD and `S…

d94c02e

…pan[Scalar[D]]` (#52584)

[External] [stdlib] Add `List[Scalar[D]].extend()` from `SIMD` and
`Span[Scalar[D]]`

Add `List[Scalar[D]].extend()` from `SIMD` and `Span[Scalar[D]]`

Split off from #3814. This is needed to enable efficient
appending of scalar value sequences to a `List` without having to resort
to `UnsafePointer` manually.

ORIGINAL_AUTHOR=martinvuyk
<110240700+martinvuyk@users.noreply.github.com>
PUBLIC_PR_LINK=#3854

Co-authored-by: martinvuyk <110240700+martinvuyk@users.noreply.github.com>
Co-authored-by: Connor Gray <code@connorgray.com>
Closes #3854
MODULAR_ORIG_COMMIT_REV_ID: 7d0c724497ba0671ae660f4de5758d6c4baad7bc

martinvuyk mentioned this pull request

[stdlib] Hristo/foundations for os process module #4017

Open

martinvuyk added 2 commits

February 25, 2025 13:26


          Merge remote-tracking branch 'upstream/main' into make-list-size-private

44625b9


          fix detail

fc70e1e

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

Collaborator

ConnorGray commented Feb 25, 2025

!sync

modular-automation bot assigned ConnorGray

modularbot added the imported-internally label

martinvuyk added 2 commits

February 26, 2025 10:31


          Merge remote-tracking branch 'upstream/main' into make-list-size-private

f67cc53


          fix iadd length to use byte_length

ffab93a

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

martinvuyk requested a review from a team as a code owner

February 26, 2025 13:32


          revert: fix iadd length to use byte_length

0b12853

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>

Contributor Author

martinvuyk commented Feb 26, 2025

I didn't request a review :o . Just edited something that was a silly mistake

JoeLoser approved these changes

View reviewed changes

Collaborator

JoeLoser commented Feb 26, 2025

!sync

modular-automation bot assigned JoeLoser

Collaborator

JoeLoser commented Feb 26, 2025

Quick update here: I've fixed (almost) all of our internal use so this should land soon internally. I'm OOO tomorrow, so if it doesn't land tonight/soon, it will land Friday. Thanks, Martin!

JoeLoser removed the waiting for response label

Collaborator

modularbot commented Feb 27, 2025

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the main branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

modularbot added merged-internally merged-externally labels

Collaborator

modularbot commented Feb 27, 2025

Landed in 5f2570f! Thank you for your contribution 🎉

modularbot closed this in

5f2570f

martinvuyk deleted the make-list-size-private branch

February 27, 2025 20:03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

imported-internally merged-externally merged-internally