-
-
Notifications
You must be signed in to change notification settings - Fork 594
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 reverse #1082
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
@steff456 Looks like the tests don't cover all branches for reversing code points. Would you mind adding a few tests to address? |
You can generate a test coverage report locally using |
Ran One can see the uncovered lines online as well: https://coverage.stdlib.io/string/base/reverse-code-point/ |
Thanks @Planeshifter and @kgryte, I'll check the tests :) |
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'm not sure why the coverage tests failed with the last commit, but when I try it locally it is working 🙃 |
@steff456 I believe the test coverage failure is due to a known issue. @Planeshifter can you comment? |
@steff456 Will look into it, sorry for that; seems like the job was not able to push the coverage report to the test coverage repo. The table now shows that 100% coverage is achieved, though, so that's awesome! |
Perf! Thanks to both 👏🏻 |
@steff456 Would you mind renaming the packages from
to
The use of singular is a bit confusing in this context, as, e.g., |
Coverage Report
The above coverage report was generated for the changes in this PR. |
lib/node_modules/@stdlib/string/base/reverse-code-points/examples/index.js
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-code-points/lib/main.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-code-points/test/test.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-code-points/test/test.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-code-points/test/test.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-grapheme-clusters/README.md
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-grapheme-clusters/README.md
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/reverse-grapheme-clusters/README.md
Outdated
Show resolved
Hide resolved
Coverage Report
The above coverage report was generated for the changes in this PR. |
lib/node_modules/@stdlib/string/base/reverse-grapheme-clusters/README.md
Outdated
Show resolved
Hide resolved
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.
@steff456 Thanks for working on this. Left a few comments. The handling of grapheme clusters looks off to me, based on the expected test values. Additionally, we need to add a couple more TypeScript tests and fix some missing documentation.
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. |
for ( i = idx; i < brk; i++ ) { | ||
cluster += str.charAt( i ); | ||
} | ||
out = cluster + out; |
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.
For future reference, this is not the most memory efficient approach (i.e., prepending strings), but we don't have much choice until the bug in prevGraphemeClusterBreak
is resolved (ref: #1092).
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.
LGTM. Thanks, @steff456!
Coverage Report
The above coverage report was generated for the changes in this PR. |
The failing CI jobs do not appear related to this PR. As such, will go ahead and merge... |
Part of #1062
Description
This pull request:
@stdlib/string/reverse
@stdlib/string/base/reverse
@stdlib/string/base/reverse-code-points
@stdlib/string/base/reverse-grapheme-clusters
Related Issues
This pull request:
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers