String.find/String.rfind USize proposal
#216
Conversation
ponylang-main
added
discuss during sync
chalcolith
added
status - final comment period
|
@hovsater can you rebase this against main? |
Done! |
text/0000-usize-indexing
Outdated
|
|
||
| # How We Teach This | ||
|
|
||
| N/A |
There was a problem hiding this comment.
We are teaching this in the same way we are currently teaching. Via the standard library documentation. This N/A should be changed. It is applicable and there is a means that we are already teaching such things.
Small change but important for not setting an incorrect precedent going forward.
There was a problem hiding this comment.
I think this could be that we already teach the usage of rfind and find via the method documentation and that nothing needs to change regarding that. And that otherwise, inclusion in the release notes plus examples of how to update code where a type annotation is given and the lack of a change if type implication is used would suffice here.
We do however, I think want release notes that detail the change, that it is breaking and how to fix it if the break impacts on you. The fix in this case might be "obvious" but I think it behooves us to practice giving explanations for fixing any breaking changes we introduce.
There was a problem hiding this comment.
Thanks for the feedback. I've addressed that in 6746310.
|
|
||
| # How We Test This | ||
|
|
||
| Standard CI and the compiler should identify most areas that need to be addressed. Additionally, any documentation, code examples, and guides should be reviewed and updated to reflect the new behavior. |
There was a problem hiding this comment.
Such a review should happen before this RFC is accepted as fixing anything that this breaks that isn't programmatically caught should be part of the acceptance criteria for merging the change to the return type.
There was a problem hiding this comment.
There's no usage in the tutorial that I can find.
There was a problem hiding this comment.
There's no usage in pony-patterns that I can find.
There was a problem hiding this comment.
The only usage I can find on the website is a reference to this RFC from the most recent LWIP.
There was a problem hiding this comment.
So what should I write here instead, @SeanTAllen?
text/0000-usize-indexing
Outdated
|
|
||
| # Unresolved questions | ||
|
|
||
| N/A |
There was a problem hiding this comment.
This is applicable.
Can you change this to "none at this time". I believe that is what you meant.
However, given the state of this RFC, unresolved at this time is anything that needs to be updated that this might break in documentation.
There was a problem hiding this comment.
Another thought. Given that this will break existing code that might require changes in other ponylang org projects, I'd like to have encoded into this RFC or at least an agreement amongst committers that we will not merge this change without an assurance from the author of the PR that implements this to quickly address all breakages in ponylang org libraries that our nightly breakage tests identify (if any exist).
I've often been left to pick up that important piece and I'd like to have a discussion that about how we address that isn't solely "Sean labor".
There was a problem hiding this comment.
This is applicable.
Can you change this to "none at this time". I believe that is what you meant.
However, given the state of this RFC, unresolved at this time is anything that needs to be updated that this might break in documentation.
See 6746310.
|
I'm in favor of this RFC with the changes I have requested. |
|
The next few sync calls where we would vote on this are cancelled due to holidays, so we decided to approve this RFC today, conditional on Sean's concerns above being addressed to his satisfaction. So we don't need to wait for a future vote - this can get merged as soon as those changes have come in and been approved by Sean. |
The current return type of
String.findandString.rfindisISize, which stems from its historical behavior of returning-1when no match was found. However, the methods now returnerrorfor such cases. ReturningUSizemakes more sense because indices are always non-negative, and it aligns withArray.find's method signature.