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

[Doc] Update rustdoc #44

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Conversation

apepkuss
Copy link
Collaborator

@apepkuss apepkuss commented Aug 1, 2023

In this PR, the rustdoc is updated.

Copy link
Member

juntao commented Aug 1, 2023

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


Overall, the patch mostly consists of documentation updates and minor improvements. There are a few potential issues and errors that should be addressed:

  1. In the executor.rs file, the documentation comments have been changed to use inconsistent terms like "wasi instance" and "import module" instead of the previously used "import object". It's recommended to revert these changes and maintain consistency by using the original term "import object".

  2. In the module.rs file, the documentation comment has been updated to refer to "import module" instead of "import object". However, based on the context and usage in other parts of the codebase, it seems like this change is not accurate. It's recommended to revert this change and continue using "import object".

In addition to these potential issues, the following findings are worth mentioning:

  1. In the README.md file, some of the links have been updated, but there are issues with the correctness and verification of these links. Specifically, the link to the WasmEdge documentation has been removed and a new link has been added, which currently points to the same URL as the WasmEdge website. These changes should be reviewed and corrected if necessary.

  2. In the patch that adds a new step to build and deploy the Async API document, there are a few aspects that require attention. Firstly, it's unclear why the Async API document needs to be published separately, and this purpose and usage should be documented. Additionally, there should be comments explaining the necessity of enabling certain features in the cargo doc command. The condition for deploying the Async API document should be verified, and the option to force_orphan in the deployment steps should be carefully considered to prevent overwriting existing documents. Lastly, a more descriptive commit message would be helpful to understand the purpose of these changes.

In summary, while the patch mainly consists of documentation updates and minor improvements, there are some issues, such as inconsistent terms and incorrect or unverified links, that should be addressed. Additionally, the new steps related to the Async API document building and deployment require further clarification and consideration.

Details

Commit f3cb78acf752bcb4c309d731ec068d2d6ba8f6a6

Key changes in the patch:

  • Updated documentation comments in the executor.rs and module.rs files.

Potential problems:

  • The documentation comment in executor.rs was changed to refer to a "wasi instance" instead of an "import object". This may cause confusion as it doesn't align with the function signature and the term used in other parts of the codebase. It's recommended to revert this change and keep the original term "import object".
  • The documentation comment in executor.rs was changed to refer to an "import module" instead of an "import object". This change is inconsistent with the rest of the codebase, where "import object" is used for the same concept. It's recommended to revert this change and keep using "import object".
  • The documentation comment in module.rs was updated to refer to "import module" instead of "import object". Based on the context and the usage of the term "import object" in other parts of the codebase, it seems like this change is not accurate. It's recommended to revert this change and keep using "import object".

Overall, the patch updates the documentation comments, but there are some changes that should be reverted to ensure consistency and clarity in the codebase.

Commit 82de152773a88cc3391f790f891469b1ccdd316d

The key changes in this patch are:

  • The README.md file has been updated with new links and some text changes.
  • The async.rs file has been updated with documentation comments for the types and methods.
  • The io.rs file has a small change in the documentation comments for the params! macro.
  • The lib.rs file has a small change in the documentation comments.

Potential problems:

  • The pull request author has updated the links in the README.md file, but it seems that some of the links are incorrect or need further verification. For example, the link to the WasmEdge documentation in the README.md file has been removed, and a new link to the WasmEdge documentation has been added, but it is currently pointing to the same URL as the WasmEdge website. These changes should be reviewed and corrected if necessary.

Other than that, the changes seem to be mostly documentation updates and minor improvements, which are generally safe.

Commit 1bdc58cf02fab60333d26decd6e9805a47f7055f

Key changes:

  • Added a new step to build the Async API document for the wasmedge-rust-sdk project.
  • Added a new step to deploy the Async API document to the gh-pages branch.

Potential problems:

  • It's unclear why the Async API document needs to be published separately from the regular API document. The purpose and usage of the Async API document should be documented.
  • The cargo doc command in the new step has several features enabled (aot, async, wasi_crypto, wasi_nn, wasmedge_process, ffi). It would be helpful to include a comment explaining why these features are necessary for generating the Async API document.
  • The condition for deploying the Async API document is github.ref == 'refs/heads/main'. It's worth verifying if this condition is correct and aligned with the intention of the change.
  • The force_orphan option is set to true in both deployment steps. This could potentially cause the overwriting of existing documents on the gh-pages branch. It's important to ensure that this is the desired behavior.
  • It would be helpful to have a more descriptive commit message explaining the purpose of the changes made in this patch.

Commit f3b29f3eaba171030d10fda5c13e520776e9358a

Key changes:

  • The code in src/async.rs has been formatted.
  • There are no functional changes in the code.

Potential problems:

  • No potential problems were identified in this patch.

@apepkuss apepkuss requested a review from L-jasmine August 1, 2023 06:13
@apepkuss
Copy link
Collaborator Author

apepkuss commented Aug 1, 2023

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit d719b24 into WasmEdge:main Aug 1, 2023
16 of 19 checks passed
@apepkuss apepkuss deleted the doc/update-rustdoc branch August 1, 2023 06:34
@juntao juntao mentioned this pull request Aug 1, 2023
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