feat(ecmascript): String.prototype.split()

[oscarotero]

Please, bear with me. I'm learning Rust and I don't have much experience with the borrow checker.

[oscarotero]

@aapoalas I think it's ready to review.

btw, what is the recommended way to test the function? I'd like to have a file to execute (like cargo run -- eval testing.js) to manually test the function, but there isn't a console.log() that I can use to see the result.

[aapoalas]

nova_cli has a print function you can use.

[aapoalas]

Oh, oops. I only now realized this is actually your first PR here! Sorry, I wasn't paying attention and thought you were behind #416 as well!

Welcome, and great work getting yourself up from the ground with the borrow checker! And sorry about the nitpicking on the spec text: It's just a good idea to stick fairly close to it. I don't mean it personally <3

[oscarotero]

Thanks for your review @aapoalas
Writing a JS engine is harder than I thought :D

I'm learning the codebase of the repo, how to implement and test things, and now I just learned that the numbers in the comments corresponds to the steps in the ecma262 specs.

I'll continue working on this.

[aapoalas]

I'm learning the codebase of the repo, how to implement and test things, and now I just learned that the numbers in the comments corresponds to the steps in the ecma262 specs.

I'm sorry! This is definitely my failing! I should add some more about this to the CONTRIBUTING.md and maybe elsewhere as well so that new contributors have an easier way in.

[oscarotero]

Thanks for all your help @aapoalas , I'm learning a lot

There're still much work to do yet. There are some cases that are not explained in the spec but they are failing in the tests.

For example, when the this variable is not a string, I didn't see anything in the spec to explain how to handle that, and the test are not clear. I'll continue the next week.

[aapoalas]

Found a bunch to comment on, but nothing terribly out of whack. Thank you very much, and welcome again to Nova!

Absolutely feel free to keep on trying to solve any remaining cases: The eval-test command is very useful for testing individual test262 case. eg.

cargo build && cargo run --bin test262 eval-test built-ins/String/prototype/split/argument-is-new-reg-exp-and-instance-is-string-hello.js

[oscarotero]

@aapoalas I have fixed a lot of tests due a stupid bug: I was getting the limit argument with args.get(0) instead of args.get(1). The only failing tests are those related with regexp, Symbol.split and other advanced edge cases.

I've also applied your suggestions. The only unresolved comment is the one related with UTF-16.

[aapoalas]

@aapoalas I have fixed a lot of tests due a stupid bug: I was getting the limit argument with args.get(0) instead of args.get(1). The only failing tests are those related with regexp, Symbol.split and other advanced edge cases.

I've also applied your suggestions. The only unresolved comment is the one related with UTF-16.

Sorry, it's taking me some time to get to re-reviewing this. I've been buried under an avalanche of grapes during the last few days.

[oscarotero]

No problem! I saw the pictures and completely understand your priorities! 🍇

[aapoalas]

Minor nitpicks / thoughts / questions.

But looks good to me! Thank you for this, great work!

[aapoalas]

Thank you again and great work! <3