second-state / WasmEdge-WASINN-examples

Apache License 2.0
217 stars 36 forks source link

Replace unwrap() with expect() and provide error messages. #46

Closed LiyanJin closed 3 months ago

juntao commented 7 months ago

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


Overall Summary:

The pull request replaces the use of unwrap() with expect() in the set_input() and compute() methods, and also provides error messages. This improves error handling by providing more meaningful error messages when an error occurs.

There are no significant problems with this patch. However, it is recommended to provide more specific error messages to further enhance troubleshooting. Additionally, the usage of the is_empty() method in the if statement should be reviewed to ensure it is appropriate for the context and logic of the code. Overall, these changes promote good coding practices and improve error handling.

Details

Commit 6e54dd77ff37cdc5ca9b21f29c3d20233b766711

Key Changes:

Potential Problems:

Commit 8eff7d6b706c4e699f6c7187f8b7d84f7200b89f

Key changes:

Potential problems:

LiyanJin commented 5 months ago

I've updated lines 35, 36 and 41.

hydai commented 4 months ago

Hi @LiyanJin Could you please rebase the commits with the latest master branch so I can merge this PR?

LiyanJin commented 3 months ago

Hi @LiyanJin Could you please rebase the commits with the latest master branch so I can merge this PR?

Thx for reminding me. I'm going to close this PR and open one new PR based on the latest master branch.

LiyanJin commented 3 months ago

Hi @LiyanJin Could you please rebase the commits with the latest master branch so I can merge this PR?

I‘v seen you have moved wasmedge-ggml-llama-interactive example to wasmedge-ggml. And you have replaced unwrap() with expect() in /src/main.rs. So you don't need to merge my PR.