Skip to content
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: add string/base/replace-after #1363

Merged
merged 13 commits into from
Feb 25, 2024
Merged

feat: add string/base/replace-after #1363

merged 13 commits into from
Feb 25, 2024

Conversation

AuenKr
Copy link
Contributor

@AuenKr AuenKr commented Feb 24, 2024

Resolves #812 .

Description

What is the purpose of this pull request?

This pull request:

  • adding a utility to replace the substring after the first occurrence of a specified search string

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

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 added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Feb 24, 2024
@kgryte kgryte changed the title feat: add @stdlib/string/base/replace-after feat: add string/base/replace-after Feb 24, 2024
AuenKr and others added 2 commits February 24, 2024 15:33
Co-authored-by: Athan <kgryte@gmail.com>
Signed-off-by: Golden <103646877+AuenKr@users.noreply.github.com>
Signed-off-by: Athan <kgryte@gmail.com>
@kgryte
Copy link
Member

kgryte commented Feb 24, 2024

@AuenKr Would you mind updating the copyright years to 2024? Thanks!

@kgryte
Copy link
Member

kgryte commented Feb 24, 2024

@AuenKr Overall, this PR looks great! Thanks for working on this!

@kgryte
Copy link
Member

kgryte commented Feb 24, 2024

@AuenKr There's actually one change to this PR that we should make and that is support for fromIndex argument. This was previously discussed in https://github.com/stdlib-js/stdlib/pull/922/files.

The argument should be mandatory (unlike in that discussion), so the expected signature is

function replaceAfter( str, search, replacement, fromIndex ) {...}

Is this something you can include in this PR?

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Feb 24, 2024
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
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.

@AuenKr This looks good. You just need to fix the broken tests, and once finished, this should be ready for merge.

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.

LGTM. Thanks, @AuenKr, and thank you for authoring a clean PR! Will merge once CI successfully completes.

@kgryte kgryte removed the Needs Changes Pull request which needs changes before being merged. label Feb 25, 2024
@AuenKr
Copy link
Contributor Author

AuenKr commented Feb 25, 2024

@kgryte, Shouldn't string/base/replace-before also have fromIndex arguement??

Signed-off-by: Golden <103646877+AuenKr@users.noreply.github.com>
@kgryte
Copy link
Member

kgryte commented Feb 25, 2024

@AuenKr Yes, it should.

@kgryte kgryte merged commit 04ee89f into stdlib-js:develop Feb 25, 2024
7 checks passed
@AuenKr AuenKr deleted the feature/replace-after branch February 25, 2024 10:52
@AuenKr AuenKr restored the feature/replace-after branch February 25, 2024 10:52
@AuenKr AuenKr deleted the feature/replace-after branch February 25, 2024 10:52
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. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add @stdlib/string/base/replace-after
3 participants