-
Notifications
You must be signed in to change notification settings - Fork 323
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 Text.substring to allow for an easy short hand of Text.take (start.up_to end) #7913
Conversation
For #7876 adds a Text.substring function which supports negative indexes and returns a part of a string from 0-based index 'start' and continuing for 'length'
For #7876 adds a Text.substring function which supports negative indexes and returns a part of a string from 0-based index 'start' and continuing for 'length'. Also simplified get function as it looped unnecessarily.
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
…ons.enso punctuation corrections Co-authored-by: GregoryTravis <greg.m.travis@gmail.com>
Added test for start index larger than string
…g/enso into wip/cassandrac/substring
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso
Outdated
Show resolved
Hide resolved
…ons.enso updated Arguments: section to use consistent style Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
…ons.enso updated Index_Out_Of_Bounds error to reference cached length Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
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.
Looks great!
I have one thought regarding length
vs end
here, wondering if we should try to uniformize this with other similar methods (but also happy to hear why we should not 🙂).
Anyway, I think we can discuss this asynchronously and if necessary do a separate follow up PR - no need to block this one.
@@ -1791,6 +1786,35 @@ Text.parse_time_of_day self format:Date_Time_Formatter=Date_Time_Formatter.iso_t | |||
Text.parse_time_zone : Time_Zone ! Time_Error | |||
Text.parse_time_zone self = Time_Zone.parse self | |||
|
|||
## ALIAS mid, slice, substring |
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.
Regarding: slice
We already do have a slice
method on Vector
:
## GROUP Selections
Creates a new vector with the skipping elements until `start` and then
continuing until `end` index.
Arguments:
- start: The index of the first element to include.
- end: The index to stop slicing at.
> Example
Remove the first 2 elements then continue until index 5 from the vector.
[1, 2, 3, 4, 5, 6, 7, 8].slice 2 5 == [3, 4, 5]
slice : Integer -> Integer -> Vector Any
slice self start end = @Builtin_Method "Vector.slice"
I'm slightly worried that with this alias users can get confused as to what the second argument should be - in Vector.slice
that is end
but in Text.substring
(also aliased by slice
) it is length
.
Shouldn't we unify this? I was thinking that length
argument makes more sense for substring
, but actually Java's String::substring
also takes endIndex
and not length
.
And a small nitpick: please ensure to name the PR before merging it, as the name will be part of the commit history so it's good to make it something clearly readable - e.g. it could be the same/similar to the ticket name.
|
Having discussed with @radeusgd - lets mark the @Cassandra-Clark please make these changes and add a changelog entry then we should be ready to mark this PR as ready to merge. |
…g/enso into wip/cassandrac/substring
Per conversation with James, added slice back to substring
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.
Missing adding the link within the ChangeLog.md otherwise good.
Pull Request Description
Implemented substring for #7876. Also simplified get function.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR: