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

[nits] Convert request_content_len to i32 because it would be casted to i32 finally #311

Closed

Conversation

ysak-y
Copy link
Contributor

@ysak-y ysak-y commented Oct 30, 2023

What

Make type of request_content_len property in EspHttpConnection struct from u64 to i32.

Why

Now I'm working on #310 . I noticed the second argument of esp_http_client_open method expects i32 value, not u64. But request_content_len type is u64. I investigated requset_content_len property but it seems there is no reason that request_content_len type should be u64. So I want to modify it to i32 because it would make more easier to implement #310 .

When I remove as _ from https://github.com/esp-rs/esp-idf-svc/blob/master/src/http/client.rs#L251 , then shows the error as below.

      |         esp!(unsafe { esp_http_client_open(self.raw_client, self.request_content_len) })?;
      |                       --------------------                  ^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `u64`
      |                       |
      |                       arguments to this function are incorrect

How

Replace u64 type with i32 of request_content_len property. Then remove as _ syntax for them because now it has expected type.

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Oct 30, 2023

To make the process a bit simpler can you make all changes you want to do, in your initial PR with consecutively commits instead of making PR's for each commit.

@ysak-y
Copy link
Contributor Author

ysak-y commented Oct 30, 2023

IMO, PRs should be created for a single purpose. Doing so makes it easier to revert or track PRs. However, I will follow your suggestion if it is better.

@ivmarkov
Copy link
Collaborator

This PR is modifying the type of a single private variable. It does not have a purpose on its own, only as part of the other PR. Consolidate your work there please. And also follow my comments on the other PR - for example, I suggested there instead of i64 to do Option<u64>. That might or might not be a good idea. Please think about that, and try to come - in the other PR - with a complete solution implementing chunked encoding for POST/PUT requests.

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