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

String find/rfind change (RFC #80) #4587

Open
SeanTAllen opened this issue Jan 16, 2025 · 6 comments
Open

String find/rfind change (RFC #80) #4587

SeanTAllen opened this issue Jan 16, 2025 · 6 comments
Assignees
Labels
discuss during sync Should be discussed during an upcoming sync good first issue Good for newcomers

Comments

@SeanTAllen
Copy link
Member

Change the return type of String.find and String.rfind from ISize to USize, aligning its return type with Array.find.

See RFC #80 for details.

@SeanTAllen SeanTAllen added enhancement good first issue Good for newcomers help wanted Extra attention is needed labels Jan 16, 2025
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 16, 2025
@SeanTAllen
Copy link
Member Author

SeanTAllen commented Jan 16, 2025

@hovsater I want to get this in before the release at the end of the month. I am happy to do, but didn't want to "swipe it from you" if you were wanting to do. Let me know if you want to do it.

@hovsater
Copy link

Thanks for the ping, @SeanTAllen. I definitely want to do this. I'll open a draft PR shortly for the compiler itself. How do we address usage in Pony packages? I remember you said the same person should do that. Should I search for usage?

@SeanTAllen
Copy link
Member Author

We test them with each nightly for breakage, so after it's merged, within 36 hours, we'd know.

@SeanTAllen SeanTAllen removed discuss during sync Should be discussed during an upcoming sync enhancement labels Jan 21, 2025
@SeanTAllen
Copy link
Member Author

@hovsater i want to get this in by Monday to have enough time to fix breakage before the release next weekend. Can you do that or should I get this?

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 25, 2025
@hovsater
Copy link

@hovsater i want to get this in by Monday to have enough time to fix breakage before the release next weekend. Can you do that or should I get this?

Sorry for the silence. I've been super busy with work and family, so I doubt I'll have a chance to address this before end of month.

Feel free to jump on it if this is urgent. I'd be happy to finish it, but likely that would be sometime in early February.

@SeanTAllen SeanTAllen self-assigned this Jan 26, 2025
@SeanTAllen SeanTAllen removed the help wanted Extra attention is needed label Jan 26, 2025
@SeanTAllen
Copy link
Member Author

I started to do this RFC and I am pausing. We need to discuss the RFC and if we want to go forward. It might not be extensive enough. Throughout string, we have things like:

fun ref cut_in_place(from: ISize, to: ISize = ISize.max_value())

Which doesn't make a ton of sense with the find and rfind change from ISize to USize for return type. We need to review all the offset related methods (some are USize and some are ISize) and make sure we want to make this change without doing others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss during sync Should be discussed during an upcoming sync good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants