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

Document CodeBlock #1691

Merged
merged 4 commits into from
Dec 5, 2021
Merged

Conversation

TheDoctor314
Copy link
Contributor

This Pull Request fixes/closes #1609.

It changes the following:

  • Adds CodeBlock docs

@Razican
Copy link
Member

Razican commented Oct 27, 2021

Hi @TheDoctor314! Thank you for your contribution :)

This indeed goes towards the good direction. I see a couple of missing things, though. First, the length attribute doesn't have correct documentation comments (// should be changed for ///). The params attribute is also missing documentation.

This PR, in order to fully solve #1609, should also add some documentation to all internal methods where it's missing, such as new() and instruction_operands(), and to the module-level too.

Furthermore, I would prefer if we could add some extra explanation, like a longer description below what is usually the short description (the first line). And, if we could have some # Safety documentation in the Readable trait this would be perfect :)

@Razican Razican added documentation update documentation enhancement New feature or request labels Oct 27, 2021
@Razican Razican added this to the v0.14.0 milestone Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #1691 (81595ac) into main (da29677) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1691      +/-   ##
==========================================
- Coverage   50.91%   50.85%   -0.07%     
==========================================
  Files         199      199              
  Lines       17707    17803      +96     
==========================================
+ Hits         9016     9053      +37     
- Misses       8691     8750      +59     
Impacted Files Coverage Δ
boa/src/vm/code_block.rs 0.00% <ø> (ø)
boa/src/syntax/parser/function/mod.rs 49.65% <0.00%> (-12.15%) ⬇️
boa/src/syntax/ast/node/mod.rs 70.16% <0.00%> (-1.27%) ⬇️
...pression/primary/async_generator_expression/mod.rs 46.51% <0.00%> (-1.11%) ⬇️
...ser/expression/primary/generator_expression/mod.rs 43.90% <0.00%> (-1.10%) ⬇️
...arser/expression/primary/object_initializer/mod.rs 49.44% <0.00%> (-0.75%) ⬇️
boa/src/builtins/console/mod.rs 25.00% <0.00%> (-0.31%) ⬇️
...tax/parser/expression/assignment/arrow_function.rs 63.49% <0.00%> (+0.15%) ⬆️
boa/src/builtins/function/arguments.rs 85.71% <0.00%> (+0.25%) ⬆️
boa/src/object/internal_methods/function.rs 71.56% <0.00%> (+0.56%) ⬆️
... and 4 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 da29677...81595ac. Read the comment docs.

@TheDoctor314
Copy link
Contributor Author

Thanks for the feedback!
I am not too familiar with unsafe code in general. What kind of safety argument should be made for the Readable trait?

#[derive(Debug, Trace, Finalize)]
pub struct CodeBlock {
/// Name of this function
pub(crate) name: JsString,

// The length of this function.
/// The length of this function.
Copy link
Member

Choose a reason for hiding this comment

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

Is this the length of arguments this function takes?

@jasonwilliams
Copy link
Member

Maybe something like this at the top which says.

//! CodeBlock
//! This module is for the CodeBlock which implements a function representation in the VM

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

above

@jasonwilliams
Copy link
Member

And, if we could have some # Safety documentation in the Readable trait this would be perfect :)

@HalidOdat are you able to help explain the need for unsafe readable here?
https://github.com/boa-dev/boa/blob/main/boa/src/vm/code_block.rs#L26-L35

boa/src/vm/code_block.rs Outdated Show resolved Hide resolved
Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

LGTM but I think we might want to wait for @HalidOdat to say something about the documentation of the unsafe parts

@jasonwilliams jasonwilliams merged commit e1b2abb into boa-dev:main Dec 5, 2021
@TheDoctor314 TheDoctor314 deleted the code-block-docs branch December 8, 2021 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation update documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Code Block
5 participants