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

Update plugin section - user guide #146

Merged
merged 4 commits into from
Nov 20, 2023
Merged

Conversation

mhmohona
Copy link
Contributor

Explanation

Added plugin user guide for C, Rust and Go.

Related issue- #85

What type of PR is this

/kind documentation

Signed-off-by: Mahfuza <[email protected]>
Copy link
Collaborator

alabulei1 commented Aug 14, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

The patch titled "Update plugin section - user guide" includes various changes related to adding a new "Plugin-User Guide" category to the documentation and creating new files for C, Go, and Rust SDKs. There are a few potential issues and errors that need attention:

  1. None of the new files have been reviewed or tested yet, leaving room for mistakes or inaccuracies in the content.
  2. The patch does not provide updates to existing files, making it unclear if all necessary changes for supporting the new plug-in functionality have been made.
  3. Information about building or installing the plug-ins themselves is missing, which creates a documentation gap.
  4. There is no mention of any limitations or known issues with using plug-ins, which could help set appropriate user expectations.

Additionally, some important findings include:

  1. The changes introducing the Rust SDK guide and updating the C SDK guide seem to be fine and do not introduce any potential problems.
  2. The updates for the Go and Rust SDK guides include minor fixes related to headings and code block language corrections.
  3. In the code changes, there are some issues such as duplicate comments, minor formatting inconsistencies, and a misleading variable name that should be addressed.

In conclusion, while the overall changes in the patch are positive, it is important to review and test the new files thoroughly, address potential gaps in the documentation, and fix the identified issues in the code and comments.

Details

Commit 7730f0970b801c0c84a6c8a4b005e86e04a5db9d

Key changes:

  • Added a new "Plugin-User Guide" category to the documentation
  • Added new documentation files for C, Go, and Rust SDKs related to using plug-ins with the WasmEdge runtime

Potential problems:

  • None of the new files have been reviewed or tested yet, so there could be mistakes or inaccuracies in the content
  • The patch does not include any updates to existing files, so it's unclear if all necessary changes have been made to support the new plug-in functionality
  • The patch does not provide information about how to build or install the plug-ins themselves, only how to use them once they are loaded into the runtime. This could be a potential gap in the documentation.
  • There is no information about any potential limitations or known issues with using plug-ins in the WasmEdge runtime. It would be helpful to include this information to set appropriate expectations for users.

Commit a62557a5496985c078a1752495cdff47fda408c9

Key changes:

  • Added a guide for using the Rust SDK for extending the WasmEdge runtime.
  • Created new files for the Rust SDK guide.
  • Updated the C SDK guide to fix a section heading and improve readability.

Potential problems:

  • None in the provided patch.

Overall, the changes seem to be adding new content and improving the existing guide without introducing any issues.

Commit cee5ffe59e8855d2673d02c17410d300f7ac64d7

Key Changes:

  • The heading in go_sdk.md has been updated to "Using Plug-ins to Extend the Runtime in Go".
  • The heading in rust_sdk.md has been updated to "Using Plug-ins to Extend the Runtime in Rust".
  • In rust_sdk.md, the code block language has been corrected from "toml" to "rust".

Potential Problems:

  • The patch does not provide a detailed explanation of what the "small fix" is. It would be helpful to have more context on the specific changes made.
  • The patch does not include any additional changes or updates other than the language corrections and heading updates.

Overall, the patch appears to be minor and corrects some language and heading inconsistencies.

Commit f84473defb7ffe1bc6211603ebfeff05e88eeaf6

Key Changes:

  • Removed unnecessary comment blocks and corrected formatting in the code.
  • Replaced "WasiNNName" with "WasiNNModule" in the code.

Potential Problems:

  • The code comments contain duplicate information, which could lead to confusion for readers.
  • There is a minor formatting inconsistency in the code comments ("will print here" vs "will print").
  • The variable name "WasiNNName" is misleading and should be changed to "WasiNNModule" to better reflect its purpose.

Overall, the changes seem to be minor fixes and improvements to the code and comments.

@mhmohona mhmohona marked this pull request as draft August 14, 2023 06:44
Signed-off-by: Mahfuza <[email protected]>
@mhmohona mhmohona marked this pull request as ready for review August 16, 2023 10:40
@mhmohona
Copy link
Contributor Author

@alabulei1 @q82419 I have completed working on this PR. Will you please have a look at it?
Thank you!

docs/embed/use-case/plugin/go_sdk.md Outdated Show resolved Hide resolved
docs/embed/use-case/plugin/rust_sdk.md Outdated Show resolved Hide resolved
Signed-off-by: Mahfuza <[email protected]>
@mhmohona mhmohona requested review from q82419 and alabulei1 September 1, 2023 05:19
docs/embed/use-case/plugin/c_sdk.md Outdated Show resolved Hide resolved
Signed-off-by: Mahfuza <[email protected]>
@alabulei1
Copy link
Collaborator

alabulei1 commented Sep 13, 2023

I just released one question. Why this plugin guide located in the Embed section, not the extend section? @mhmohona

@mhmohona
Copy link
Contributor Author

I just released one question. Why this plugin guide located in the Embed section, not the extend section? @mhmohona

@alabulei1, I was instructed to add the user guide here. cc: @q82419

@alabulei1 alabulei1 merged commit a34fc38 into WasmEdge:main Nov 20, 2023
6 checks passed
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