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.

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] [NFC] Simplify String List[Byte] constructor #3856

Open
wants to merge 11 commits into
base: nightly
Choose a base branch
from
Open

[stdlib] [NFC] Simplify String List[Byte] constructor #3856

wants to merge 11 commits into from

Conversation

martinvuyk
Copy link
Contributor

Simplify String List[Byte] constructor

@martinvuyk martinvuyk requested a review from a team as a code owner December 10, 2024 22:59
@martinvuyk martinvuyk changed the title [stdlib] Simplify String List[Byte] constructor [stdlib] [NFC] Simplify String List[Byte] constructor Dec 10, 2024
@martinvuyk martinvuyk force-pushed the simplify-string-list-constructor branch from 778876c to e432ae3 Compare December 13, 2024 00:23
@ConnorGray
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Jan 8, 2025
@ConnorGray
Copy link
Collaborator

Hey Martin, I'm sorry to step on your toes here, but I just wanted to let you know I implemented a very similar refactoring that just merged internally.

I was in the area removing @implicit from these constructors, and noticed the same duplication you fixed here.

I'm sorry again about that, I'll keep trying to do better on noticing ahead of time when I'm overlapping a community PR😌

@martinvuyk
Copy link
Contributor Author

@ConnorGray don't worry it's no biggie. I don't know why I merged it now if you already imported it lol, but the CI is failing now. I think it's because of removing the @implicit from the String from StringSlice constructor. PR #3922 will fix this by making String.startswith() use StringSlice.

@ConnorGray
Copy link
Collaborator

Thanks for understanding Martin, and thank you for bringing that issue to my attention. I just merged your PR with the fix internally, so that should go out with the next nightly :)

@ConnorGray
Copy link
Collaborator

ConnorGray commented Jan 8, 2025 •
edited
Loading

Given the above discussion I'm going to close this PR. But feel free to re-open if I missed something or if there were pieces here you still want to see merged :)

@ConnorGray ConnorGray closed this Jan 8, 2025
@martinvuyk
Copy link
Contributor Author

@ConnorGray I can't reopen the PR and I would like to. Seeing commit 4c2c201 I don't understand why you built a whole method when rebind does the trick. There is no need to move any data if parameters are just a type system "interpretation" (casting is free): self._buffer = rebind[Self._buffer_type](buffer).

@ConnorGray ConnorGray reopened this Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsh lsh lsh left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ConnorGray ConnorGray

Labels
imported-internally Signals that a given pull request has been imported internally.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinvuyk @ConnorGray @lsh @modularbot