Skip to content

Add support for getting next extended grapheme cluster break #361

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
Merged
merged 10 commits into from
Sep 30, 2020
Merged

Add support for getting next extended grapheme cluster break #361

merged 10 commits into from
Sep 30, 2020

Conversation

@ShraddheyaS
Copy link
Contributor

@ShraddheyaS ShraddheyaS commented Sep 8, 2020 •
edited
Loading

Resolves #357.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

  • Read, understood, and followed the contributing guidelines, including the relevant style guides.
  • Read and understand the Code of Conduct.
  • Read and understood the licensing terms.
  • Searched for existing issues and pull requests before submitting this pull request.
  • Filed an issue (or an issue already existed) prior to submitting this pull request.
  • Rebased onto latest develop.
  • Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

  • Adds support for getting next extended grapheme cluster break in a string after a specified position.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

@stdlib-js/reviewers

kgryte
kgryte requested changes Sep 14, 2020
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@ShraddheyaS Overall looks good, with some minor comments, questions, and requested changes. Once we resolve these, this should be ready for merge!

@kgryte kgryte mentioned this pull request Sep 14, 2020
Closed
7 tasks
@ShraddheyaS ShraddheyaS requested a review from kgryte September 24, 2020 11:27
kgryte
kgryte approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@ShraddheyaS Wow! Really great work! Will touch up a couple of minor things on my end. Thanks for this contribution!

lib/node_modules/@stdlib/string/next-grapheme-cluster-break/README.md

- If `string` is an empty string, the function returns `-1` irrespective of `fromIndex`.
- If there is no extended grapheme cluster break after `fromIndex`, the function returns `-1`.
- Note that `fromIndex` does **not** refer to a visual character position, but to an index in the ordered sequence of [UTF-16][utf-16] code units.
Copy link
Member

Choose a reason for hiding this comment

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

Aside: we may want to consider, if we don't already have such functionality, one or more string utilities (as separate packages) for indexing, and maybe iterating over, visual characters. I believe such utilities would build on the current work.

@kgryte kgryte merged commit 1ee75fc into stdlib-js:develop Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kgryte kgryte kgryte approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RFC: Add support for getting next extended grapheme cluster break Algorithm to remove the first string character is not Unicode aware

2 participants

@ShraddheyaS @kgryte