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 missing import #1287

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Add missing import #1287

merged 1 commit into from
Nov 4, 2024

Conversation

siefkenj
Copy link
Contributor

@siefkenj siefkenj commented Nov 4, 2024

The current example provided in the documentation fails to build because it uses the wat without a reference. This PR adds the necessary use statement.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (35e75c4) to head (2ddfb8e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1287   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files         306      306           
  Lines       25277    25277           
=======================================
+ Hits        20607    20608    +1     
+ Misses       4670     4669    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Robbepop
Copy link
Member

Robbepop commented Nov 4, 2024

@siefkenj thank you for the bug report and fix. How did you discover this bug? Unfortunately the CI did not catch it. When using cargo test --doc locally it somehow also passes the test for me. 🤔

It seems that cargo implicitly adds all dev-depdendencies to all doc tests. I can also leave away the anyhow::Result import as long as I specify anyhow::Result below instead of just Result. I guess this is how doc tests are meant to work. Though this makes pasting them as examples a bit harder. That's why the Wasmi CI did not report it as an issue, because it isn't an issue for Cargo's build system.

Then again, it is even more important for me to know how you found this issue to begin with.

@Robbepop
Copy link
Member

Robbepop commented Nov 4, 2024

@siefkenj I am going to merge this out of caution but would be very happy about a response later.

@Robbepop Robbepop merged commit 45d2f90 into wasmi-labs:main Nov 4, 2024
19 checks passed
@siefkenj
Copy link
Contributor Author

siefkenj commented Nov 4, 2024

@siefkenj thank you for the bug report and fix. How did you discover this bug?

I did cargo add wasmi to another project, then I copy-and-pasted the example into it, and it failed to build.

@Robbepop
Copy link
Member

Robbepop commented Nov 4, 2024

@siefkenj thank you for the bug report and fix. How did you discover this bug?

I did cargo add wasmi to another project, then I copy-and-pasted the example into it, and it failed to build.

Ah okay, that is to be expected then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants