-
-
Notifications
You must be signed in to change notification settings - Fork 401
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
Fix: String.prototype.replace substitutions, closes #610 #629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
+ Coverage 72.50% 73.14% +0.64%
==========================================
Files 179 192 +13
Lines 13376 14005 +629
==========================================
+ Hits 9698 10244 +546
- Misses 3678 3761 +83
Continue to review full report at Codecov.
|
Multi-line description of commit, feel free to be detailed.
9c69cca
to
eb813fd
Compare
I'm not gonna be working for a week, but I found what I think was an old implementation of the method from V8 in js. From a quick glance I think we need to invoke |
I've found another bug in the implementation of `String.prototype.replace', copied from Chrome: > let str = 'boa';
> str.replace(/.*/, "??$&??")
"??boa??"
> str.replace(".*", "??$&??")
"boa" While on |
I ended up not fixing the other bug, my intention was to follow the spec, but it requires I think Another thing to keep in mind is |
When I get back I'll implement tests for PS: Just now realized that I could use tuple pattern matching instead of using |
Now covers all cases in https://tc39.es/ecma262/#table-45 except named capture groups because it's not yet implemented
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.
Introduce early return if `regexp|substr` isn't found in `this` Add test case covering this situation
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.
This is good to know from my side now!
@jasonwilliams @HalidOdat @Lan2u sorry to tag, but it's been waiting for a 2nd review for a week |
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.
This looks good! check my comments to see on how we might improve it :)
Sorry for the long delay I haven't had that much free time this week.
Sugestions by @HalidOdat in boa-dev#629
Sugestions by @HalidOdat in boa-dev#629
Sugestions by @HalidOdat in boa-dev#629
c258ebc
to
54df208
Compare
This Pull Request fixes/closes #610 .
It changes the following: