Skip to content

feat: add string/base/for-each-right #1369

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 7 commits into from
Feb 25, 2024
Merged

feat: add string/base/for-each-right #1369

merged 7 commits into from
Feb 25, 2024

Conversation

AhmedKhaled590
Copy link
Contributor

@AhmedKhaled590 AhmedKhaled590 commented Feb 24, 2024 •
edited by kgryte
Loading

Resolves #856

Description

What is the purpose of this pull request?

Adding support for invoking a callback for each UTF-16 character of a string, while iterating from right-to-left

This pull request:

  • add @stdlib/string/base/for-each-right

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

stdlib-bot
stdlib-bot reviewed Feb 24, 2024
View reviewed changes
Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@kgryte kgryte changed the title feat: add @stdlib/string/for-each-right issue#856 feat: add string/base/for-each-right Feb 24, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. Utilities Issue or pull request concerning general utilities. labels Feb 24, 2024
Jaysukh-409
Jaysukh-409 reviewed Feb 24, 2024
View reviewed changes
@AhmedKhaled590
Copy link
Contributor Author

Hi @Jaysukh-409
All Comments are resolved.

Planeshifter
Planeshifter approved these changes Feb 24, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @AhmedKhaled590 for working on this package (and to @Jaysukh-409 for reviewing!) We can merge after CI has cleared.

@Planeshifter
Update lib/node_modules/@stdlib/string/base/for-each-right/docs/types…
63ce45c 
…/index.d.ts

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update lib/node_modules/@stdlib/string/base/for-each-right/docs/repl.txt
ab8b3a2 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
lib/node_modules/@stdlib/string/base/for-each-right/docs/types/index.d.ts
*
* forEach( 'Hello, World!', log );
*/
declare function forEachRight( str: string, clbk: Callback, thisArg?: any ): string;
Copy link
Member

Choose a reason for hiding this comment

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

@Planeshifter We can improve the type for thisArg, correct? That will impact the callbacks above.

Copy link
Member

@Planeshifter Planeshifter Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kgryte Yes, specifically making use of ThisParameterType to extract the type of the 'this' parameter of the callback function type, and then using generics for the forEachRight function that will be passed to the this parameter of the callbacks. But that is currently not done in any of the string/base packages and maybe we should address it in a follow-up PR / issue?

Copy link
Member

Choose a reason for hiding this comment

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

That is fine. Would you mind creating an issue for this with a link to some sample code where we do use ThisParameterType? This could be a good first issue for contributors to work on.

Copy link
Member

Choose a reason for hiding this comment

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

@kgryte Will do!

kgryte
kgryte requested changes Feb 24, 2024
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.

A few nits, but overall looking good. Thanks for working on this @AhmedKhaled590.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Feb 24, 2024
@Planeshifter
Update lib/node_modules/@stdlib/string/base/for-each-right/docs/repl.txt
7b5bd1e 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter Planeshifter merged commit 0aa7b4a into stdlib-js:develop Feb 25, 2024
@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
@AhmedKhaled590 AhmedKhaled590 mentioned this pull request Mar 29, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stdlib-bot stdlib-bot stdlib-bot left review comments

@Jaysukh-409 Jaysukh-409 Jaysukh-409 left review comments

@Planeshifter Planeshifter Planeshifter approved these changes

@kgryte kgryte kgryte approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: Add @stdlib/string/for-each-right
5 participants
@AhmedKhaled590 @Planeshifter @kgryte @stdlib-bot @Jaysukh-409