feat: add support for replacing a substring after the first occurrence of a search string #922
Conversation
kgryte
changed the title
|
@HarshitaKalani For this implementation, you can leave out the CLI and input validation. I've updated |
|
Hey @kgryte , please have a review and lmk if anything needs to be updated. |
| @@ -0,0 +1,20 @@ | |||
| { | |||
There was a problem hiding this comment.
This file can be removed, as the package does not include a CLI.
| */ | ||
| function replaceAfter( str, replacement, search, fromIndex ) { | ||
| var idx; | ||
| if ( !isString( str ) ) { |
There was a problem hiding this comment.
You can remove input argument validation as this is a "base" package where we assume we've been provided valid inputs.
There was a problem hiding this comment.
Should I remove the validation of the 4th argument "fromIndex" too?
There was a problem hiding this comment.
Yeah, you can remove the isInteger check.
| * | ||
| * @example | ||
| * var out = replaceAfter( 'beep boop', 'foo', 'xyz' ); | ||
| * // returns '' |
There was a problem hiding this comment.
This is unexpected. I'd expect that, if a search string is not present in an input string, the function would return the input string unchanged.
| * | ||
| * @example | ||
| * var out = replaceAfter( 'beep boop', 'foo', 'beep', 5 ); | ||
| * // returns '' |
| idx = str.indexOf( search ); | ||
| } | ||
| if ( idx === -1 ) { | ||
| return ''; |
|
@kgryte please have a review. |
lib/node_modules/@stdlib/string/base/replace-after/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/string/base/replace-after/docs/repl.txt
Outdated
Show resolved
Hide resolved
|
@HarshitaKalani Am I correct in thinking that this PR is complete and ready for review? |
|
Hey @kgryte , please review it and suggest me a good issue to pick next. |
| var replaceAfter = require( '@stdlib/string/base/replace-after' ); | ||
| ``` | ||
|
|
||
| #### replaceAfter( str, replacement, search\[, fromIndex] ) |
There was a problem hiding this comment.
@HarshitaKalani I just noticed this, but the argument order is reversed from @stdlib/string/base/replace-before. Is there a reason for this? Personally, I would expect the argument order to be the same for the sake of API consistency.
There was a problem hiding this comment.
I kept it that way since there's an extra argument 'fromIndex', so thought it's better to keep search at last in case fromIndex is also given as the argument.
Should I keep it as replaceAfter(str, search, replacement[, fromIndex] ) or replaceAfter(str, search[, fromIndex], replacement) would be better?
There was a problem hiding this comment.
Given that @stdlib/string/replace is
replace( str, search, newval )
and @stdlib/string/replace-before is
replaceBefore( str, search, replacement )
My preference is to keep consistency where fromIndex is the last argument and search comes before replacement.
replaceAfter( str, search, replacement[, fromIndex] )
The fromIndex argument comes last in other APIs (e.g., @stdlib/string/starts-with and others), so IMO best to keep consistent here and have it be the last argument, which is also better from a polymorphic interface standpoint. An optional argument in the middle of mandatory arguments is generally harder to optimize.
| - If a substring is not present in a provided string, the function returns an empty string. | ||
| - If provided an empty substring, the function returns the input string. |
There was a problem hiding this comment.
These notes are not correct. See replace-before.
|
|
||
| b.tic(); | ||
| for ( i = 0; i < b.iterations; i++ ) { | ||
| out = replaceAfter( str, values[ i % values.length ], fromCodePoint( i%126 ) ); // eslint-disable-line max-len |
There was a problem hiding this comment.
Rather than use fromCodePoint, use ., as done in replace-before.
| /// <reference types="@stdlib/types"/> | ||
|
|
||
| /** | ||
| * Returns the part of a string after a specified substring. |
| */ | ||
| function replaceAfter( str, replacement, search, fromIndex ) { | ||
| var idx; | ||
| if ( arguments.length > 3 ) { |
There was a problem hiding this comment.
We should include similar checks as in replace-before. Namely, if str or search is empty, we can return an input string.
| * var out = replaceAfter( 'beep boop beep baz', 'foo', 'beep', 5 ); | ||
| * // returns 'beep boop beepfoo' | ||
| */ | ||
| function replaceAfter( str, replacement, search, fromIndex ) { |
There was a problem hiding this comment.
As discussed for the README, IMO the argument order should be updated to match replace-before.
HarshitaKalani
force-pushed
the
replace_after_2
branch
from
bfb01a2 to
18f5e8f
Compare
|
I'm getting this linting error whenever I try to commit. Please suggest how to get over with the error. |
|
Try upgrading to the latest version of Node.js (eg, v18). |
kgryte
added
Stale
Resolves #812 .
With this PR, I'm adding the implementation for replace-after issue.
@stdlib-js/reviewers