[Merged by Bors] - Implementation of JsMap Wrapper

[nekevss]

This Pull Request related to JsMap for #2098.

Any feedback on implementing JsMapIterator would be welcome. I wasn't entirely sure if it was the right approach, but as I worked on the example file, it felt like something at least similar would be needed to use Map's .entries(), .keys(), and .values() methods.

It changes the following:

• Implements JsMap Wrapper
• Implements JsMapIterator Wrapper
• Creates JsMap example in boa_examples

[codecov]

Codecov Report

Merging #2115 (18bd5f6) into main (ce8c6e2) will decrease coverage by 1.07%.
The diff coverage is 1.66%.

@@            Coverage Diff             @@
##             main    #2115      +/-   ##
==========================================
- Coverage   43.40%   42.32%   -1.08%     
==========================================
  Files         220      228       +8     
  Lines       20079    21047     +968     
==========================================
+ Hits         8715     8909     +194     
- Misses      11364    12138     +774     

Impacted Files	Coverage Δ
boa_engine/src/object/jsmap.rs	0.00% <0.00%> (ø)
boa_engine/src/object/jsmap_iterator.rs	0.00% <0.00%> (ø)
boa_engine/src/object/mod.rs	20.78% <ø> (+0.28%)	⬆️
boa_engine/src/object/jsobject.rs	52.46% <50.00%> (-0.17%)	⬇️
...ax/ast/node/declaration/async_function_decl/mod.rs	52.63% <0.00%> (-14.04%)	⬇️
.../syntax/parser/expression/left_hand_side/member.rs	26.92% <0.00%> (-12.21%)	⬇️
...src/syntax/parser/expression/left_hand_side/mod.rs	45.45% <0.00%> (-11.69%)	⬇️
boa_engine/src/syntax/parser/mod.rs	26.78% <0.00%> (-9.98%)	⬇️
boa_engine/src/builtins/promise/mod.rs	18.97% <0.00%> (-9.71%)	⬇️
boa_engine/src/syntax/ast/node/identifier/mod.rs	18.18% <0.00%> (-9.10%)	⬇️
... and 67 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce8c6e2...18bd5f6. Read the comment docs.

[raskad]

I think this looks really good. The implementation of JsMapIterator looks totally logical to me. Is there a reason forEach is missing?

One little nitpick; there are some comments that don't start uppercase and end with a dot. Just for consistency and style ;)

And we probably need a rebase, because the 262 submodule seems to be outdated here.

[nekevss]

I can clean up the comments real quick. They were mostly guideposts I was using while going.

There isn't much of a reason that forEach was left off. To be honest, I wasn't entirely certain about adding it as it seemed to be omitted from JsArray, which mostly made me pause and leave it off. I can definitely add forEach.

The 262 submodule is probably totally my bad. Any guidance on how I can fix it on my end would be really appreciated!

[raskad]

The 262 submodule is probably totally my bad. Any guidance on how I can fix it on my end would be really appreciated!

You can update the 262 submodule with git submodule update --init. You probably have to checkout main, pull the current changes, update the submodule there. Then you can rebase your branch on main and update the submodule there too. If I'm not mistaken that should do it.

[nekevss]

@raskad I updated the comments and the submodule in main and ran the rebase. There were also some changes that needed to be made to .keys(), .entries(), and .values() since IteratorRecord had changed in one of the recent pull requests.

The submodule still didn't want to update on the branch with the git submodule update --init command, which was one of the issues I was having. I basically ended up going into test262 and pulling from main there. So hopefully that should be fixed too.

[nekevss]

I made some pretty big changes to the comments and documentation for JsMap, building out some of the comments and documentation/examples where possible. The documentation do vary between shorthand code snippets and longer examples, but they may have caused the continuous integration check fails. Do code snippets in comments typically cause these tests to fail or should the shorthand examples be lengthened?

Also, I did shy away from going too deep into the documentation surrounding JsMapIterator as I believe the next method returns an IteratorResult. I wasn't certain what the discussion was around IteratorResult and whether that should be exposed or handled within the next method of JsMapIterator, so I thought it best to leave it open for now

[jedel1043]

@nekevss Very nice work!

The thing about Rust examples in docs is that cargo's test framework will try to compile and test them to verify that they're valid.
This means you need to create a context for each example, just like the examples inside the struct doc.
Also, if you want to hide the initialization code, check https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#hiding-portions-of-the-example for code snippets on how to do that :D

[jedel1043]

Very very great work! I'm very impressed with the addition of so many example usages. I just have a single nit. After that, it's ready to merge.

[jedel1043]

Looks good to me!

[jedel1043]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• Clippy
• Documentation
• Examples
• Rustfmt
• Tests on Linux
• Tests on MacOS
• Tests on Windows