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

Add usage example for JsPromise wrapper #2804

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lvllvl
Copy link

@lvllvl lvllvl commented Apr 11, 2023

This Pull Request fixes/closes #2761 .

It adds the following:

  • a new file --> boa_examples/src/bin/jspromise.rs
  • the code example is taken from boa_engine/src/object/builtins/jspromise.rs (line 115-155)

boa_examples/src/bin/jspromise.rs Outdated Show resolved Hide resolved
builtins::promise::PromiseState,
Context, JsValue, js_string};

fn main() -> Result<(), Box<dyn Error>> {
Copy link
Member

Choose a reason for hiding this comment

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

The example looks good, but looking at other examples, we should be adding more comments through the code, explaining what each step does.

Also, is there any other API usage we can show?

Copy link
Author

Choose a reason for hiding this comment

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

Added comments to the code.

Do you mean that this example should use more of the api or that there should be another jspromise example in addition to this one?

Copy link
Member

Choose a reason for hiding this comment

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

Probably that the example should use more methods of JsPromise.

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #2804 (1dc7414) into main (24a4770) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 1dc7414 differs from pull request most recent head 4153843. Consider uploading reports for the commit 4153843 to get more accurate results

@@            Coverage Diff             @@
##             main    #2804      +/-   ##
==========================================
- Coverage   51.30%   51.28%   -0.02%     
==========================================
  Files         416      417       +1     
  Lines       41193    41204      +11     
==========================================
  Hits        21133    21133              
- Misses      20060    20071      +11     
Impacted Files Coverage Δ
boa_examples/src/bin/jspromise.rs 0.00% <0.00%> (ø)

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

@jedel1043 jedel1043 requested review from a team and removed request for Razican, jasonwilliams, HalidOdat, RageKnify, raskad, jedel1043 and nekevss May 5, 2023 14:26
@Razican Razican changed the title Add uses example for JsPromise wrapper Add usage example for JsPromise wrapper May 5, 2023
@jedel1043 jedel1043 added the waiting-on-author Waiting on PR changes from the author label Nov 29, 2023
@jedel1043
Copy link
Member

@lvllvl Do you have any progress for this? If you don't have the time to work on this, that's also okay :)

@lvllvl lvllvl marked this pull request as draft December 22, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting on PR changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add usage example for JsPromise wrapper
3 participants