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

use wasmedge_hyper_rustls #16

Merged
merged 1 commit into from
Aug 20, 2023
Merged

Conversation

L-jasmine
Copy link
Contributor

No description provided.

Signed-off-by: csh <458761603@qq.com>
Copy link
Member

juntao commented Aug 20, 2023

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


.gitignore

The provided code is not sufficient to provide a complete review. Could you please provide the source code file and its patch so that I can review it properly?

The key change in the provided patch is the addition of a new line that includes */target after /target in the code. This indicates a modification in the file or directory path matching pattern being used.

client-https/Cargo.toml

The code appears to be a Rust project configuration file, specifically a Cargo.toml file. It defines dependencies for the project, including versions and features.

Here are my comments on the code:

  1. The version of the "hyper_wasi" dependency is specified as "0.15" with the "full" feature. Ensure that this specific version and feature are required for the project.

  2. The version of the "http-body-util" dependency is specified as "0.1.0-rc.2". Make sure this specific version is necessary and compatible with other dependencies.

  3. The version of the "tokio_wasi" dependency is specified as "1" with several features, including "rt", "macros", "net", "time", and "io-util". Verify that these features are required for the project and ensure compatibility with other dependencies.

  4. The version of the "pretty_env_logger" dependency is specified as "0.4.0". Confirm if this specific version is necessary and compatible with other dependencies.

  5. The version of the "wasmedge_rustls_api" dependency is specified as "0.1" with the "tokio_async" feature. Ensure that this version and feature combination is required for the project.

  6. The "wasmedge_hyper_rustls" dependency is specified as "0.1.0". Check if this specific version is necessary and compatible with other dependencies.

Overall, the code does not have any apparent problems. However, it's important to carefully review the versions and features of the dependencies to ensure they are required and compatible with each other.

The key change in the patch is the addition of the "wasmedge_hyper_rustls" dependency with a version specified as "0.1.0".

client-https/src/main.rs

Overall, the code looks fine. However, there are a few potential problems and improvements that can be made:

  1. Error handling: The code currently uses unwrap() in a few places. It would be better to handle errors properly using Result and ? operator. This way, if any errors occur, they can be propagated and handled gracefully.

  2. Error type: The current Result type is using Box<dyn std::error::Error + Send + Sync>. While this is a valid choice, it might be helpful to use a more specific error type that provides additional information about the error.

  3. Type alias: The Result type alias is not necessary as it is only used once. It can be removed.

  4. Dependency: The code appears to be missing the necessary imports for the wasmedge_hyper_rustls and wasmedge_rustls_api crates. Make sure they are included and added as dependencies in the project's Cargo.toml file.

Apart from these points, the code seems to be following good practices and using the hyper library correctly.

Key changes in the patch:

  1. Import changes:

    • Import statement for Request and TcpStream have been removed.
    • Import statement for hyper::Client has been added.
  2. Connection handling changes:

    • Instead of establishing the connection using TcpStream, the code now uses wasmedge_hyper_rustls::connector::new_https_connector to create an HTTPS connector with the given wasmedge_rustls_api::ClientConfig.
    • The client is built using Client::builder().build method with the HTTPS connector.
  3. Request and response changes:

    • The code removes the manual creation of the request using Request::builder() and the send and receive process using sender.send_request.
    • The code now uses the client.get method to send a GET request.

Overall, the patch simplifies the code by replacing the manual connection handling and request creation with the use of the hyper library's Client and get method.

Copy link
Member

@juntao juntao left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks.

@juntao juntao merged commit 1f9d8cc into WasmEdge:main Aug 20, 2023
2 checks passed
@juntao juntao deleted the bump_to_hyper_rustls branch August 20, 2023 17:17
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.

None yet

2 participants