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.

Sign up for GitHub

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

Jump to bottom

Text.take and Text.drop #3287

Merged
merged 32 commits into from
Feb 22, 2022
Merged

Text.take and Text.drop #3287

merged 32 commits into from
Feb 22, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Feb 18, 2022 •
edited
Loading

Pull Request Description

Implementation of the Text take and drop APIs

  • Added Range.contains function
  • Added Text_Sub_Range type
  • Added Text_Utils.index_of and Text_Utils.last_index_of based on ICU StringSearcher

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@jdunkerley jdunkerley force-pushed the wip/jd/text-take-drop-181265131 branch from 340fc72 to bd8e72a Compare February 21, 2022 10:51
@jdunkerley
Adding Range.contains
ea962db
@jdunkerley
Adding Runtime.lazy
66f2c77 
Fix Range
First n vaguely works
@jdunkerley
First, Last, Before and After working
987202d 
Some expansion to Text_Utils
@jdunkerley
Removing Runtime.lazy
11a2121 
Hiding the utility function inside
@jdunkerley
Support for While
e1fb16d 
Starting on take function
@jdunkerley
Support for Range for take
a6616c0
@jdunkerley
Move to Range over Pair for internals
6c49709 
Adding drop support
@jdunkerley
Working drop and take
aa26c94 
Some completed tests
@jdunkerley
Emoji tests
bc7ed35 
Out of range tests
Starting on accents
@jdunkerley jdunkerley force-pushed the wip/jd/text-take-drop-181265131 branch from b7b844f to bc7ed35 Compare February 21, 2022 16:44
jdunkerley and others added 5 commits February 21, 2022 17:38
@jdunkerley
Take accent tests
2b3c078 
Adjust index_of to use StringSearcher
Fix for failing While
@jdunkerley
Drop on graphemes.
481bd60 
Still to fix not found scenarios in drop
@jdunkerley
More tests
c584926
@jdunkerley
Final tests and changelog
6a96756
@jdunkerley
PR comment
71d2ac2
@jdunkerley jdunkerley marked this pull request as ready for review February 21, 2022 19:21
@jdunkerley jdunkerley requested a review from 4e6 as a code owner February 21, 2022 19:21
radeusgd
radeusgd reviewed Feb 22, 2022
View reviewed changes
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 good, but I would suggest a few minor changes.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Range.enso Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Range.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Range.enso Outdated Show resolved Hide resolved
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
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Text_Sub_Range.enso Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Text_Sub_Range.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Text_Sub_Range.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Range_Spec.enso Show resolved Hide resolved
radeusgd
radeusgd approved these changes Feb 22, 2022
View reviewed changes
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 good now, although I'd consider also using the breakIterator.next count trick for First and Last on to_char_range.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Feb 22, 2022
radeusgd
radeusgd reviewed Feb 22, 2022
View reviewed changes
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Text_Sub_Range.enso
Comment on lines 75 to 79
first_count = text.length - count
indices = find_sub_range_end text (index->_->_-> index+1 == first_count)
if indices.first == -1 then (Range 0 indices.second) else
(Range indices.second (Text_Utils.char_length text))
iterator = BreakIterator.getCharacterInstance
iterator.setText text
start_index = iterator.next first_count
Range (if start_index == -1 then 0 else start_index) (Text_Utils.char_length text)
Copy link
Member

@radeusgd radeusgd Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here technically it may be faster to do:

iterator.last
start_index = iterator.next -count

iterator.last is an O(1) operation so it will be faster than our .length. But it's a minor detail.

@mergify mergify bot merged commit 2e2c556 into develop Feb 22, 2022
@mergify mergify bot deleted the wip/jd/text-take-drop-181265131 branch February 22, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@4e6 4e6 4e6 approved these changes

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdunkerley @4e6 @radeusgd