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

Add removeprefix/removesuffix to string #212

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Feb 15, 2022

The names and behavior of the new functions removeprefix and removesuffix are consistent with PEP 616.

Fixes #185.

@fmeum
Copy link
Contributor Author

fmeum commented Feb 15, 2022

@adonovan I added the new methods in alphabetical order rather than adjacent to endswith. Please let me know if I should change that.

@fmeum
Copy link
Contributor Author

fmeum commented Feb 15, 2022

The CI fails with with an errors that seems unrelated:

Error in fail: error running 'git fetch origin master:origin/master' while working with @starlark-rust:
fatal: Couldn't find remote ref master

Copy link
Contributor

@laurentlb laurentlb left a 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks wrong

Copy link
Contributor Author

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.

@fmeum
Copy link
Contributor Author

fmeum commented Feb 15, 2022

The change looks good to me.

Edit: Implementation in Java: bazelbuild/bazel#14824

According to #185 (comment), they are also available in Rust Starlark.

Edit: They indeed are since facebook/starlark-rust@7a7ef4b.

@laurentlb
Copy link
Contributor

I think we need to fix the tests (in a separate PR) before we can merge this.

(or we'll need an admin)

Copy link
Contributor

@adonovan adonovan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fmeum
Copy link
Contributor Author

fmeum commented Feb 15, 2022

@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?

@laurentlb
Copy link
Contributor

Done.

Thanks a lot for your help!

@tetromino
Copy link
Collaborator

tetromino commented Feb 23, 2022

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 foo.removesuffix(".so") += ".dll" to sometimes mutate foo and sometimes not?

@tetromino tetromino reopened this Feb 23, 2022
@adonovan
Copy link
Contributor

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 foo.removesuffix(".so") += ".dll" to sometimes mutate foo and sometimes not?

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 prefix, if S starts with that prefix, otherwise it returns S.

@tetromino tetromino closed this Feb 23, 2022
@laurentlb
Copy link
Contributor

to sometimes mutate foo and sometimes not?

Strings are immutable in Starlark.

@fmeum fmeum deleted the removeprefix branch February 24, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add removeprefix/removesuffix to string
4 participants