-
Notifications
You must be signed in to change notification settings - Fork 163
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
Add removeprefix/removesuffix to string #212
Conversation
@adonovan I added the new methods in alphabetical order rather than adjacent to |
The CI fails with with an errors that seems unrelated:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me.
Edit: Implementation in Java: bazelbuild/bazel#14824
spec.md
Outdated
```python | ||
"banana".removeprefix("ban") # "ana" | ||
"banana".removeprefix("foo") # "banana" | ||
"foofoobar".removeprefix("foobar") # "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, got argument and return value mixed up. Should be fixed now.
According to #185 (comment), they are also available in Rust Starlark. Edit: They indeed are since facebook/starlark-rust@7a7ef4b. |
I think we need to fix the tests (in a separate PR) before we can merge this. (or we'll need an admin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@adonovan @laurentlb Only just noticed that there is #193, which has more details and added the methods to one more ToC. Should that one be merged instead? |
bd2786d
to
5e07323
Compare
Done. Thanks a lot for your help! |
The spec change contradicts PEP 616: if the string does not have a prefix/suffix, we want to (as in PEP 616) return a copy of the string, not the original string. Do we really want |
Starlark doesn't have a notion of string identity, so you can't distinguish the original from a copy. That said, it would be more precise to express it as: S.removeprefix(prefix) returns the portion of S after the prefix |
Strings are immutable in Starlark. |
The names and behavior of the new functions
removeprefix
andremovesuffix
are consistent with PEP 616.Fixes #185.