Add head and tail arguments to chop #24126
Conversation
base/strings/util.jl
Outdated
| if head == 0 | ||
| tail == 0 && return SubString(s) | ||
| return SubString(s, 1, prevind(s, endof(s), tail)) | ||
| end |
There was a problem hiding this comment.
If would find it more readable with an else block and a return so that the two cases are completely parallel.
| julia> chop(a, 1, 2) | ||
| "ar" | ||
|
|
||
| julia> chop(a, 5, 5) |
There was a problem hiding this comment.
The description should mention that no error is thrown if there are not enough characters.
There was a problem hiding this comment.
Added. This is the current behavior as chop("") returns "".
base/strings/util.jl
Outdated
| # negative values of head/tail will throw error in nextind/prevind | ||
| if head == 0 | ||
| tail == 0 && return SubString(s) | ||
| return SubString(s, 1, prevind(s, endof(s), tail)) |
There was a problem hiding this comment.
AFAIK we use start(s) instead of 1 in the existing code, though I suspect we should move to 1 everywhere.
There was a problem hiding this comment.
changed to start. There is a mixed style in the code base (I have seen both during the last month :)), but I agree that it is better to use start. I fix some more situations like this in strings/util.jl.
NEWS.md
Outdated
| @@ -249,6 +249,9 @@ This section lists changes that do not have deprecation warnings. | |||
| Library improvements | |||
| -------------------- | |||
|
|
|||
| * The function `chop` now accepts two arguments `head` and `tail` allowing to specify | |||
| number of characters to remove from the head and tail tail of the string ([#24126]). | |||
base/strings/util.jl
Outdated
| # negative values of head/tail will throw error in nextind/prevind | ||
| if head == 0 | ||
| if tail == 0 | ||
| return SubString(s) |
There was a problem hiding this comment.
Wouldn't it be simpler to compute the start and end indices in the branches, and call SubString only once at the end? SubString will call endof anyway if you don't do it yourself. That should make the function body smaller and allow inlining when appropriate.
There was a problem hiding this comment.
an excellent suggestion. Thank you.
base/strings/util.jl
Outdated
|
|
||
| Remove the last character from `s`. | ||
| Remove first `head` and last `tail` characters from `s`. |
There was a problem hiding this comment.
I'd have said "the first" and "the last". It would also make sense to continue saying what the default does for clarity, since that's a standard function.
base/strings/util.jl
Outdated
| @@ -94,10 +118,10 @@ julia> chomp("Hello\\n") | |||
| """ | |||
| function chomp(s::AbstractString) | |||
| i = endof(s) | |||
| (i < 1 || s[i] != '\n') && (return SubString(s, 1, i)) | |||
| (i < start(s) || s[i] != '\n') && (return SubString(s, start(s), i)) | |||
There was a problem hiding this comment.
Better leave this for another PR as it's unrelated. Anyway it's not worth changing these if we don't do it everywhere.
|
rebased |
This PR implements the second part of the proposal #23765.