WasmEdge / wasmedge-rust-sdk

Embed WasmEdge functions in a Rust host app
Apache License 2.0
29 stars 15 forks source link

[feat] Implement `FromStr` trait for `NNPreload` in `wasmedge-sdk` #81

Closed apepkuss closed 11 months ago

apepkuss commented 11 months ago

In this PR, implement the FromStr trait for the NNPreload type. The changes in this PR fixed #79.

juntao commented 11 months ago

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


Summary:

The patch adds the implementation of the FromStr trait for the NNPreload struct in the wasmedge-sdk crate, along with the necessary unit test and supporting functions. It also includes a code change to check the format of the input string and return an error if it does not meet the expected format.

The potential issues found in the patch include:

  1. The unit test only covers valid preload strings, so it would be beneficial to add tests for invalid preload strings to ensure proper error handling.
  2. The from_str function assumes a preload string will always have four parts separated by :, and it will panic if a different format is passed. It would be better to handle invalid formats gracefully and return an error instead.

The most important finding is that the patch adds input validation to the from_str function for NNPreload, ensuring that the expected format is followed and returning an error if it is not.

Overall, the changes appear to be reasonable, but it is recommended to address the potential issues mentioned above to improve the code's robustness and error handling.

Details

Commit 74a49e126278beb235238b9dd83d931b9d76b9a7

Key changes in the patch:

Potential problems:

Commit 444e8d98d40ea92fb1ef09e3da1539715ef61ad2

Key changes:

Potential problems:

Overall, the changes appear to be reasonable and add input validation to the from_str function for NNPreload.

apepkuss commented 11 months ago

@L-jasmine Could you please help review this PR? Thanks a lot!