-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
feat: refactor string package remove-last #1079
Conversation
lib/node_modules/@stdlib/string/base/remove-last-code-point/lib/main.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/remove-last-code-point/test/test.js
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/remove-last-code-point/test/test.js
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/remove-last/docs/types/index.d.ts
Outdated
Show resolved
Hide resolved
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @steff456. Left a few comments regarding the handling of code points and grapheme clusters. Once these are resolved, should be ready for merge.
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
i = 0; | ||
num = 0; | ||
while ( num < total - n ) { | ||
i = nextGraphemeClusterBreak( str, i ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works and should be fine for now. Leaving a comment for future reference that this is a two-pass algorithm. Once we update prev-grapheme-cluster-break
, we should be able to implement as a one-pass algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steff456 All review comments have now been addressed. This is ready for merge.
Coverage Report
The above coverage report was generated for the changes in this PR. |
Part of #1062
Description
This pull request:
@stdlib/string/remove-last
@stdlib/string/base/remove-last
@stdlib/string/base/remove-last-code-point
@stdlib/string/base/remove-last-grapheme-cluster
package.json
in@stdlib/string/base/remove-first
,@stdlib/string/base/remove-first-code-point
,@stdlib/string/base/remove-first-grapheme-cluster
Related Issues
This pull request:
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers