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 Text.substring to allow for an easy short hand of Text.take (start.up_to end) #7913

Merged
merged 11 commits into from
Sep 29, 2023

Conversation

Cassandra-Clark
Copy link
Contributor

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:

  • All code follows the guide
  • All code has been tested:
    • Unit tests have been written where possible.

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.
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2023

CLA assistant check
All committers have signed the CLA.

Cassandra-Clark and others added 3 commits September 27, 2023 13:38
…ons.enso


punctuation corrections

Co-authored-by: GregoryTravis <greg.m.travis@gmail.com>
Added test for start index larger than string
Cassandra-Clark and others added 2 commits September 28, 2023 07:19
…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>
Copy link
Member

@radeusgd radeusgd left a 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
Copy link
Member

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.

@radeusgd
Copy link
Member

radeusgd commented Sep 28, 2023

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.

Edit: typo 😅

@Cassandra-Clark Cassandra-Clark changed the title Wip/cassandrac/substring Add Text.substring to allow for an easy short hand of Text.take (start.up_to end) Sep 28, 2023
@jdunkerley
Copy link
Member

Having discussed with @radeusgd - lets mark the slice method on Vector/Array as PRIVATE to avoid confusion.
We can remove the alias to slice on the new substring method as well.

@Cassandra-Clark please make these changes and add a changelog entry then we should be ready to mark this PR as ready to merge.

Copy link
Member

@jdunkerley jdunkerley left a 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.

@Cassandra-Clark Cassandra-Clark merged commit d7258ab into develop Sep 29, 2023
27 checks passed
@Cassandra-Clark Cassandra-Clark deleted the wip/cassandrac/substring branch September 29, 2023 16:57
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 Text.substring to allow for an easy short hand of Text.take (start.up_to end)
5 participants