Skip to content
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

[Merged by Bors] - Separate JsObjectType implementors to their own module #2324

Closed
wants to merge 5 commits into from

Conversation

CalliEve
Copy link
Contributor

@CalliEve CalliEve commented Oct 3, 2022

This Pull Request closes #2080.

It moves all implementors of the JsObjectType trait into their own js_object module.
This should simplify documentation and by doing a pub(crate) export in object little to no imports within the crate need to be changed, simplifying the usage of this module within the boa_engine crate.
Documentation within the object module has been updated to reflect this change and in a way that it is shown on the home page of the documentation.

@CalliEve CalliEve marked this pull request as draft October 3, 2022 11:38
@CalliEve
Copy link
Contributor Author

CalliEve commented Oct 3, 2022

Temporarily converting this back to a draft while I figure out why the compilation now fails, after merging in the changes from main.
Fixed the issues caused by the addition of jsdataview and a mistake with the import of jsset in the merge conflict resolution

@CalliEve CalliEve marked this pull request as ready for review October 3, 2022 12:32
@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Merging #2324 (0e79248) into main (eaf9428) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2324   +/-   ##
=======================================
  Coverage   40.93%   40.93%           
=======================================
  Files         238      238           
  Lines       22509    22509           
=======================================
  Hits         9214     9214           
  Misses      13295    13295           
Impacted Files Coverage Δ
boa_engine/src/object/builtins/jsarray.rs 5.15% <ø> (ø)
boa_engine/src/object/builtins/jsarraybuffer.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsdataview.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsfunction.rs 40.00% <ø> (ø)
boa_engine/src/object/builtins/jsmap.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsmap_iterator.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsproxy.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsset.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jsset_iterator.rs 0.00% <ø> (ø)
boa_engine/src/object/builtins/jstypedarray.rs 0.00% <ø> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@CalliEve
Copy link
Contributor Author

CalliEve commented Oct 6, 2022

@jedel1043 if you have time, could you review this PR? And is there anything else I should do? 😅
I ask because I am also wondering if I interpreted the issue correctly, or if I should change some stuff in the PR

@jedel1043 jedel1043 added documentation update documentation API hacktoberfest-accepted PR accepted for Hacktoberfest labels Oct 6, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Oct 6, 2022
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! And sorry for the delay 😅. The PR looks good! However, I would lift JsObject to be inside the object module, since that's the main object users would interact with, and change js_object to something like builtins, just to clarify that these are wrappers for builtin objects.

Also, can you make the new module a proper directory? It would be very good to have the wrappers on a separate directory from the main object internals.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me as soon as @jedel1043's proposal is implemented

@CalliEve CalliEve requested review from jedel1043 and removed request for HalidOdat and raskad October 8, 2022 13:12
@CalliEve
Copy link
Contributor Author

CalliEve commented Oct 8, 2022

I implemented the changes requested by @jedel1043. Let me know if there's anything else I should do 😄
(somehow me re-requesting a review from @jedel1043 removed the other review requests, whoops 😓)

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, looks perfect!

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 8, 2022
This Pull Request closes #2080.

It moves all implementors of the `JsObjectType` trait into their own `js_object` module.
This should simplify documentation and by doing a `pub(crate)` export in `object` little to no imports within the crate need to be changed, simplifying the usage of this module within the boa_engine crate.
Documentation within the `object` module has been updated to reflect this change and in a way that it is shown on the home page of the documentation.
@bors
Copy link

bors bot commented Oct 8, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Separate JsObjectType implementors to their own module [Merged by Bors] - Separate JsObjectType implementors to their own module Oct 8, 2022
@bors bors bot closed this Oct 8, 2022
@RageKnify RageKnify added the Internal Category for changelog label Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API documentation update documentation hacktoberfest-accepted PR accepted for Hacktoberfest Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate JsObjectType implementors to their own module
4 participants