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

Added boa examples #634

Closed
wants to merge 0 commits into from
Closed

Added boa examples #634

wants to merge 0 commits into from

Conversation

elasmojs
Copy link
Contributor

This Pull Request fixes/closes #446 .

It changes the following:

  • root cargo.toml to add examples to the workspace
  • created examples folder with content
  • added load from string
  • added load from file
  • added handling return val using forward_val
  • added global object additions
  • added a custom module handler example

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #634 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #634   +/-   ##
=======================================
  Coverage   72.81%   72.81%           
=======================================
  Files         179      179           
  Lines       13197    13197           
=======================================
  Hits         9609     9609           
  Misses       3588     3588           

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 b8d0b3c...7dad1a4. Read the comment docs.

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 for the contribution! I added some suggestions where I found them reasonable.

I think it would be best if we used normal Cargo examples for the examples, instead of creating a new package.

In any case, about the examples, I didn't have the time to check them thoroughly, but there are some things missing:

  • I noticed no \n at the end of many files. This doesn't seem to be following the .editorConfig file in the project. Most code editors should use it by default.
  • We should add CI for all these examples.
  • I'm missing more module-level documentation in each example file.

examples/scripts/calc.js Outdated Show resolved Hide resolved
examples/scripts/helloworld.js Outdated Show resolved Hide resolved
examples/scripts/calctest.js Outdated Show resolved Hide resolved
examples/scripts/enhancedglobal.js Outdated Show resolved Hide resolved
examples/src/enhancedglobal.rs Outdated Show resolved Hide resolved
//Read the module source file
println!("Loading: {}", libfile);
let buffer = read_to_string(libfile);
if buffer.is_err(){
Copy link
Member

Choose a reason for hiding this comment

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

As clippy suggests, we should use if...let here.

examples/src/modulehandler.rs Outdated Show resolved Hide resolved
let return_value = ResultValue::from(Ok(Value::from(module_exports)));
return return_value;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

examples/src/returnval.rs Outdated Show resolved Hide resolved
examples/Cargo.toml Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added API enhancement New feature or request labels Aug 15, 2020
@elasmojs
Copy link
Contributor Author

elasmojs commented Aug 16, 2020

@HalidOdat Changed to engine.to_string as suggested in #633

I am able to run it in the dev branch but the CI fails at the following code snippets. Request help thanks.

Pattern 1
engine.to_string(arg).unwrap()
| ^^^^^^^^^ method not found in &mut boa::exec::Interpreter

Pattern 2
let result = arg0.to_integer() + arg1.to_integer();
| ^^^^^^^^^^- supplied 0 arguments
| |
| expected 1 argument

@HalidOdat
Copy link
Member

HalidOdat commented Aug 16, 2020

Pattern 1
engine.to_string(arg).unwrap()
| ^^^^^^^^^ method not found in &mut boa::exec::Interpreter

it should be the other way around arg.to_string(&mut engine).unwrap()

Pattern 2
let result = arg0.to_integer() + arg1.to_integer();
| ^^^^^^^^^^- supplied 0 arguments

The previous implementation of Value::to_integer did not take a context, this is not spec compliant, so to fix this you need to do arg.to_integer(&mut engine)

@Razican
Copy link
Member

Razican commented Aug 19, 2020

I think this should be blocked until #627 lands. That will give us much better examples.

@Razican Razican added blocked Waiting for another code change documentation update documentation labels Aug 19, 2020
@RageKnify RageKnify removed the blocked Waiting for another code change label Dec 10, 2020
@Razican
Copy link
Member

Razican commented Jan 11, 2021

Hi @elasmojs, how is this going?

@jasonwilliams
Copy link
Member

oops, rebased it and it somehow lost the changes here.
I think because the issue here closed (due to there being no changes) ive lost access to @elasmojs so i can't change it.

I've created a new PR

bors bot pushed a commit that referenced this pull request Mar 8, 2022
Added boa examples as per issue #446
Overtaken #634

Somehow screwed that branch up by rebasing it and losing access
pings @elasmojs 

This Pull Request fixes/closes #446 .


Co-authored-by: Jason Williams <jase.williams@gmail.com>
Co-authored-by: Iban Eguia (Razican) <razican@protonmail.ch>
Co-authored-by: jasonwilliams <jase.williams@gmail.com>
Co-authored-by: jedel1043 <jedel0124@gmail.com>
otravidaahora2t added a commit to otravidaahora2t/boa that referenced this pull request Aug 2, 2024
Added boa examples as per issue #446
Overtaken boa-dev/boa#634

Somehow screwed that branch up by rebasing it and losing access
pings @elasmojs 

This Pull Request fixes/closes #446 .


Co-authored-by: Jason Williams <jase.williams@gmail.com>
Co-authored-by: Iban Eguia (Razican) <razican@protonmail.ch>
Co-authored-by: jasonwilliams <jase.williams@gmail.com>
Co-authored-by: jedel1043 <jedel0124@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API documentation update documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add examples
5 participants