first and last with nchar #23960
first and last with nchar #23960
Conversation
base/strings/basic.jl
Outdated
| """ | ||
| first(str::AbstractString, nchar::Integer) | ||
|
|
||
| Get a string consisting of first `nchar` characters of `str`. |
NEWS.md
Outdated
| @@ -236,8 +236,12 @@ This section lists changes that do not have deprecation warnings. | |||
| Library improvements | |||
| -------------------- | |||
|
|
|||
| * Functions `first` and `last` now accept `nchar` argument for `AbstractString`. | |||
| If this argument is used they reutrn a substring consisting of first/last `nchar` | |||
There was a problem hiding this comment.
"substring" could be confused with SubString.
There was a problem hiding this comment.
replaced "substring" with "string"
NEWS.md
Outdated
| @@ -236,8 +236,12 @@ This section lists changes that do not have deprecation warnings. | |||
| Library improvements | |||
| -------------------- | |||
|
|
|||
| * Functions `first` and `last` now accept `nchar` argument for `AbstractString`. | |||
| If this argument is used they reutrn a string consisting of first/last `nchar` | |||
|
I have improved the implementation. The old version were not type stable for |
|
It seems weird to add these for strings but not for arrays and other iterable collections. |
| return str[1:nchar] | ||
| end | ||
| str[1:nextind(str, 1, nchar-1)] | ||
| end |
There was a problem hiding this comment.
My recommendation in #23765 is that not. Usually first and last would be used with a small nchar so there is not much gain from SubString (or actually a loss if original string is very large, we get only a small part of it and the original string can be discarded). Observe that it can be a SubString if the original string is a SubString.
|
@stevengj regarding adding |
|
@stevengj and @fredrikekre are we satisfied with this? I can't see anything that wasn't addressed (but I'm tired) and it's been open a long time. |
| if 0 <= nchar <= 1 | ||
| return str[1:nchar] | ||
| end | ||
| str[1:nextind(str, 1, nchar-1)] |
There was a problem hiding this comment.
This seems like it will give a BoundsError for nchar > length(str). Shouldn't it be min(endof(str), nextind(str, 1, nchar-1))?
There was a problem hiding this comment.
Or is BoundsError intended here?
There was a problem hiding this comment.
It was intended. The idea is to have an invariant length(first(s, nchar)) == nchar. Similarly for last.
This might be changed the way you propose - then also docstring should be changed as it now promises nchar characters in the produced string.
I slightly prefer the current implementation (as it ensures the invariant) but I can see the rationale behind your proposal so I am OK change it.
There was a problem hiding this comment.
If we changed it to "at most" nchar elements of the string, then it would be similar to take.
My feeling is that using "at most" nchar elements would be a big more flexible, and it wouldn't hurt the uses where you want exactly nchar elements except that it wouldn't throw an error for a string of the wrong size.
There was a problem hiding this comment.
OK. I will make a PR to have a clear decision point.
Implements new functionality of
firstandlastforAbstractStringas discussed in #23765.