feat(string): add normalize method #441
Conversation
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Hello and welcome! First off, great work and thank you very much: Aside from one issue with unwrapping when the spec says to rethrow an error, and the dependency definition location, this looks fully mergeable as-is which is absolutely great!
You'll need to update the test262 expectations & metrics: The magic spells for doing this are mentioned in the contributing doc under https://github.com/trynova/nova/blob/main/CONTRIBUTING.md#tests-in-prs
Again, great work, the code is really pretty and I have no real significant complaints. Stellar work! <3
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
Outdated
Show resolved
Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
Show resolved
Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
Show resolved
Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
Outdated
Show resolved
Hide resolved
nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
Outdated
Show resolved
Hide resolved
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Still has unnecessary heap data clone.
…/nova into feat/string-prototype-normalize
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Looks excellent to me now! Thank you again, welcome, and great work! <3
2 participants
Hey!
This is my attempt to implemente the
String.prototype.normalizemethod - any feedback is really appreciated.