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

Mention public API in readme #682

Merged
merged 1 commit into from
Jan 18, 2021
Merged

Mention public API in readme #682

merged 1 commit into from
Jan 18, 2021

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Jan 7, 2021

No description provided.

@gumb0 gumb0 requested review from axic and chfast January 7, 2021 12:00
@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #682 (8d5ed1a) into master (63ed27c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #682   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          72       72           
  Lines       10257    10257           
=======================================
  Hits        10187    10187           
  Misses         70       70           
Flag Coverage Δ
spectests 91.50% <ø> (ø)
unittests 99.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -42,8 +42,7 @@ $ cmake ..
$ cmake --build .
```

This will build Fizzy as a library and since there is no public API
(the so called *embedder API* in WebAssembly) yet, this is not very useful.
This will build Fizzy as a library. [C API] is provided for embedding Fizzy engine in applications, as well as [Rust binding].
Copy link
Member

Choose a reason for hiding this comment

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

I think for Rust we should give an actual example using the published crate, and perhaps link to crates.io and/or docs.rs (for documentation).

README.md Outdated
@@ -42,8 +42,7 @@ $ cmake ..
$ cmake --build .
```

This will build Fizzy as a library and since there is no public API
(the so called *embedder API* in WebAssembly) yet, this is not very useful.
This will build Fizzy as a library. [C API] is provided for embedding Fizzy engine in applications, as well as [Rust binding].
Copy link
Member

Choose a reason for hiding this comment

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

For C perhaps having a simple example would be useful, just to give an indication. Something which loads a wasm file and executes ta function called "main"?

Copy link
Member

Choose a reason for hiding this comment

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

We also have the how to include fizzy in cmake. We have a test for that.

Copy link
Member

Choose a reason for hiding this comment

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

@gumb0 perhaps this cmake example would be useful. See test/integration/cmake_package/use_fizzy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure yet how to describe it other than mentioning find_package(fizzy CONFIG REQUIRED). Can look into it later, or @chfast perhaps could help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a sentence anyway.

@gumb0 gumb0 marked this pull request as draft January 7, 2021 18:11
README.md Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Jan 8, 2021

@gumb0 do you want to drop the rust part? I can create a PR and deal with some rust description later.

@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 8, 2021

@gumb0 do you want to drop the rust part? I can create a PR and deal with some rust description later.

Yes, would be good.

@gumb0 gumb0 force-pushed the capi-readme branch 2 times, most recently from b3fecf1 to 2eee52e Compare January 8, 2021 18:16
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the capi-readme branch 2 times, most recently from d0e333a to b6782c0 Compare January 13, 2021 10:57
@gumb0 gumb0 marked this pull request as ready for review January 13, 2021 10:58
@gumb0 gumb0 requested review from axic and chfast January 13, 2021 11:00
```c
#include <fizzy/fizzy.h>

bool execute_main(const uint8_t* wasm_binary, size_t wasm_binary_size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to create matching integration test at some point to make sure this example continue to work.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

But can you rename the commit log to not include rust?

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
@gumb0 gumb0 deleted the capi-readme branch January 18, 2021 11:40
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.

3 participants