Skip to content

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

Merged
Planeshifter merged 7 commits into
stdlib-js:developfrom
AhmedKhaled590:add-string-base-for-each-right
Feb 25, 2024
Merged

feat: add string/base/for-each-right#1369
Planeshifter merged 7 commits into
stdlib-js:developfrom
AhmedKhaled590:add-string-base-for-each-right

Conversation

@AhmedKhaled590
Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/README.md Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/benchmark/benchmark.js Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/docs/types/index.d.ts Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/docs/types/test.ts Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/examples/index.js Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/lib/index.js Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/lib/main.js Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/test/test.js Outdated
@AhmedKhaled590
Copy link
Copy Markdown
Contributor Author

AhmedKhaled590 commented Feb 24, 2024

Hi @Jaysukh-409
All Comments are resolved.

Planeshifter
Planeshifter approved these changes Feb 24, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread lib/node_modules/@stdlib/string/base/for-each-right/docs/repl.txt Outdated
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/docs/types/index.d.ts Outdated
@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
Comment thread 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
Copy Markdown
Member

@kgryte kgryte Feb 24, 2024

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
Copy Markdown
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
Copy Markdown
Member

@kgryte kgryte Feb 24, 2024

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
Copy Markdown
Member

@Planeshifter Planeshifter Feb 24, 2024

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 reviewed Feb 24, 2024
View reviewed changes
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/lib/index.js Outdated
kgryte
kgryte reviewed Feb 24, 2024
View reviewed changes
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/package.json Outdated
kgryte
kgryte requested changes Feb 24, 2024
View reviewed changes
Copy link
Copy Markdown
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
Planeshifter approved these changes Feb 25, 2024
View reviewed changes
Comment thread lib/node_modules/@stdlib/string/base/for-each-right/docs/repl.txt Outdated
@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

@Planeshifter Planeshifter Planeshifter approved these changes

@kgryte kgryte kgryte approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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