Skip to content

feat: refactor truncate string package #1097

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

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

feat: refactor truncate string package #1097

wants to merge 18 commits into from

Conversation

steff456
Copy link
Contributor

@steff456 steff456 commented Sep 13, 2023

Part of #1062

Description

What is the purpose of this pull request?

This pull request:

  • Refactors the string package @stdlib/string/truncate
  • Adds @stdlib/string/base/truncate
  • Adds @stdlib/string/base/truncate-code-point
  • Adds @stdlib/string/base/truncate-grapheme-cluster
  • Adds @stdlib/string/num-code-points

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.

Checklist

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


@stdlib-js/reviewers

Sorry, something went wrong.

@steff456 steff456 requested a review from kgryte September 13, 2023 21:26
@steff456 steff456 self-assigned this Sep 13, 2023
@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-point 165 / 165
+ 100.00 %
15 / 15
+ 100.00 %
1 / 1
+ 100.00 %
165 / 165
+ 100.00 %
string/base/truncate-grapheme-cluster 142 / 142
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
142 / 142
+ 100.00 %
string/base/truncate 113 / 113
+ 100.00 %
8 / 8
+ 100.00 %
1 / 1
+ 100.00 %
113 / 113
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

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.

@steff456 Similar to string/reverse (pr: #1082), would you mind renaming truncate-code-point and truncate-grapheme-cluster to truncate-code-points and truncate-grapheme-clusters, respectively? As in that PR, it makes sense to use the plural; otherwise, the package name suggests that one truncates a single code point or grapheme cluster, which is not the case.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 165 / 165
+ 100.00 %
15 / 15
+ 100.00 %
1 / 1
+ 100.00 %
165 / 165
+ 100.00 %
string/base/truncate-grapheme-clusters 142 / 142
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
142 / 142
+ 100.00 %
string/base/truncate 113 / 113
+ 100.00 %
8 / 8
+ 100.00 %
1 / 1
+ 100.00 %
113 / 113
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 165 / 165
+ 100.00 %
15 / 15
+ 100.00 %
1 / 1
+ 100.00 %
165 / 165
+ 100.00 %
string/base/truncate-grapheme-clusters 142 / 142
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
142 / 142
+ 100.00 %
string/base/truncate 113 / 113
+ 100.00 %
8 / 8
+ 100.00 %
1 / 1
+ 100.00 %
113 / 113
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 165 / 165
+ 100.00 %
15 / 15
+ 100.00 %
1 / 1
+ 100.00 %
165 / 165
+ 100.00 %
string/base/truncate-grapheme-clusters 142 / 142
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
142 / 142
+ 100.00 %
string/base/truncate 113 / 113
+ 100.00 %
8 / 8
+ 100.00 %
1 / 1
+ 100.00 %
113 / 113
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 165 / 165
+ 100.00 %
15 / 15
+ 100.00 %
1 / 1
+ 100.00 %
165 / 165
+ 100.00 %
string/base/truncate-grapheme-clusters 142 / 142
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
142 / 142
+ 100.00 %
string/base/truncate 113 / 113
+ 100.00 %
8 / 8
+ 100.00 %
1 / 1
+ 100.00 %
113 / 113
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 162 / 162
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
162 / 162
+ 100.00 %
string/base/truncate-grapheme-clusters 139 / 139
+ 100.00 %
13 / 13
+ 100.00 %
1 / 1
+ 100.00 %
139 / 139
+ 100.00 %
string/base/truncate 112 / 112
+ 100.00 %
7 / 7
+ 100.00 %
1 / 1
+ 100.00 %
112 / 112
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 162 / 162
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
162 / 162
+ 100.00 %
string/base/truncate-grapheme-clusters 139 / 139
+ 100.00 %
13 / 13
+ 100.00 %
1 / 1
+ 100.00 %
139 / 139
+ 100.00 %
string/base/truncate 112 / 112
+ 100.00 %
7 / 7
+ 100.00 %
1 / 1
+ 100.00 %
112 / 112
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 162 / 162
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
162 / 162
+ 100.00 %
string/base/truncate-grapheme-clusters 139 / 139
+ 100.00 %
13 / 13
+ 100.00 %
1 / 1
+ 100.00 %
139 / 139
+ 100.00 %
string/base/truncate 112 / 112
+ 100.00 %
7 / 7
+ 100.00 %
1 / 1
+ 100.00 %
112 / 112
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 161 / 161
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
161 / 161
+ 100.00 %
string/base/truncate-grapheme-clusters 139 / 139
+ 100.00 %
13 / 13
+ 100.00 %
1 / 1
+ 100.00 %
139 / 139
+ 100.00 %
string/base/truncate 112 / 112
+ 100.00 %
7 / 7
+ 100.00 %
1 / 1
+ 100.00 %
112 / 112
+ 100.00 %
string/num-code-points 137 / 137
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
137 / 137
+ 100.00 %
string/truncate 187 / 187
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
187 / 187
+ 100.00 %

The above coverage report was generated for the changes in this PR.

}
data = data.toString();
if ( flags.lines ) {
lines = data.split( RE_EOL );
Copy link
Member

Choose a reason for hiding this comment

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

kgryte and others added 2 commits November 10, 2023 13:58

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
string/base/truncate-code-points 161 / 161
+ 100.00 %
14 / 14
+ 100.00 %
1 / 1
+ 100.00 %
161 / 161
+ 100.00 %
string/base/truncate-grapheme-clusters 139 / 139
+ 100.00 %
13 / 13
+ 100.00 %
1 / 1
+ 100.00 %
139 / 139
+ 100.00 %
string/base/truncate 112 / 112
+ 100.00 %
7 / 7
+ 100.00 %
1 / 1
+ 100.00 %
112 / 112
+ 100.00 %
string/num-code-points 132 / 132
+ 100.00 %
11 / 11
+ 100.00 %
1 / 1
+ 100.00 %
132 / 132
+ 100.00 %
string/truncate 186 / 186
+ 100.00 %
22 / 22
+ 100.00 %
1 / 1
+ 100.00 %
186 / 186
+ 100.00 %

The above coverage report was generated for the changes in this PR.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Athan <kgryte@gmail.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Athan <kgryte@gmail.com>
// Check for an unpaired surrogate at the end of the input string...
if ( i === len - endingLength - 1 ) {
// We found an unpaired surrogate...
break;
Copy link
Member

Choose a reason for hiding this comment

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

@steff456 Given that we break here, does this mean we could end up with a string having fewer than the desired number of Unicode code points?

The comment on L96 says the end of the input string, but this is not precise, as it is at the end of the "eligible" string. There could be a low surrogate at the next code unit, but we don't process that character, and I believe we should be.

Is my understanding correct?


str = '𐒻𐓟𐒻𐓟𐒻𐓟𐒻𐓟𐒻𐓟';
len = 5;
expected = '𐒻...';
Copy link
Member

Choose a reason for hiding this comment

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

This test case seems odd to me. Isn't the 𐒻 character comprised of a high and low surrogate? If so, that is a single Unicode code point, but the expected output is only 4 code point (visual) characters. Is this the intended behavior?


str = '\uD800𐓟𐒻𐓟𐒻𐓟𐒻𐓟𐒻𐓟';
len = 4;
expected = '...';
Copy link
Member

Choose a reason for hiding this comment

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

In this case, we are omitting the unpaired surrogate entirely, resulting in a string of 3 Unicode code points, which is less than len. This also doesn't make sense to me.

I would think that we should treat unpaired high surrogates as simply another code unit. In which case, for this test, I'd expect \uD800....

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. Needs Changes Pull request which needs changes before being merged. labels Feb 23, 2024
@headlessNode
Copy link
Member

After a discussion during project office hours on unblocking the #5414, I will split this PR it into separate PRs in order to unblock the related ongoing work.

cc @kgryte

@kgryte
Copy link
Member

kgryte commented Feb 25, 2025

@headlessNode That sounds good! Will be good to move this work forward. During review and merge, I'll be sure to give @steff456 proper attribution.

kgryte pushed a commit that referenced this pull request Mar 15, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
PR-URL: #5456
Ref: #1062
Ref: #1097
Co-authored-by: Stephannie Jiménez Gacha <steff456@hotmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com>
Signed-off-by: Muhammad Haris <harriskhan047@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants