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

New Text.insert function #3311

Merged
merged 7 commits into from
Mar 4, 2022

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Mar 2, 2022

Pull Request Description

The change adds Text.insert function to allow for adding a section of text into another at a specified position.
Implements https://www.pivotaltracker.com/n/projects/2539304.

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.

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.

Good general shape, but the current implementation seems to insert at codepoint positions whereas we use grapheme-cluster based indexing everywhere.

Great that there are tests for edge cases like inserting at the end of the string and just after it. But some more tests should be added to test the behaviour in presence of grapheme clusters that consist of multiple codepoints.

@hubertp hubertp force-pushed the wip/hubert/text-insert-181265049 branch from 80eec62 to 0d2b1b7 Compare March 3, 2022 09:16
@hubertp hubertp requested a review from radeusgd March 3, 2022 09:17
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 to me, just a few very minor comments.

test/Tests/src/Data/Text_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Text_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Text_Spec.enso Outdated Show resolved Hide resolved
hubertp and others added 3 commits March 3, 2022 13:13
Co-authored-by: Radosław Waśko <wasko.radek@gmail.com>
Addressed the remaining PR review comments
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Mar 4, 2022
@mwu-tow mwu-tow merged commit 8bdca89 into enso-org:develop Mar 4, 2022
@hubertp hubertp mentioned this pull request Mar 4, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants