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

feat: add functions to return the list of headers and their contents #681

Closed
wants to merge 1 commit into from

Conversation

nponsard
Copy link

This PR adds a function to get a reference to the list of the headers stored in the Response.

Fixes #655.

Let me know if you have any remarks.

I was also thinking about having a function return a map where the index is the name of the header, but with this PR it can be easily done afterwards (outside of this library).

@johan-bjareholt
Copy link
Contributor

@nponsard Considering the following change, I assume that this PR is not relevant anymore? 4ddcc3a

@nponsard
Copy link
Author

Ah yes, this commit breaks what I've done.

It would be easy to make the Header struct public again, but that depends on the objectives of this project / what the maintainers want.

@algesten
Copy link
Owner

Headers in ureq public API are &str for both key and value. The Header struct is internal and I don't think we should make it public here, because if it's public in one part of the API, why isn't it used everywhere?

The variant of this PR I would consider would be headers() -> (String, String) (or maybe header_iter() -> impl Iterator<Item = (&str, &str)> if we want to avoid allocation)

@nponsard nponsard force-pushed the header-list branch 2 times, most recently from 78dd306 to ed8f0f5 Compare January 15, 2024 13:51
headers() returns a vec of headers of a Response (doing allocations).
headers_iter() returns an iterator over the headers of a Response (no
allocations).
headers_hashmap() returns a hashmap of headers of a Response (doing
allocations).

Signed-off-by: Nils Ponsard <[email protected]>
@nponsard nponsard changed the title feat: add headers() function on the Response feat: add functions to return the list of headers and their contents Jan 15, 2024
@nponsard
Copy link
Author

Headers in ureq public API are &str for both key and value. The Header struct is internal and I don't think we should make it public here, because if it's public in one part of the API, why isn't it used everywhere?

* https://docs.rs/ureq/latest/ureq/struct.Response.html#method.header

* https://docs.rs/ureq/latest/ureq/struct.Request.html#method.set

The variant of this PR I would consider would be headers() -> (String, String) (or maybe header_iter() -> impl Iterator<Item = (&str, &str)> if we want to avoid allocation)

Hello !
I changed the content of my commit to account for these design decisions :

  • headers() -> (String, Option<String>)
  • headers_iter() -> impl Iterator<Item = (&str, Option<&str>)>
  • headers_hasmap() -> HashMap(String, Option<String>)

I'm not sure if the last one is useful here since it could be implemented by the user using headers_iter(). I can remove it if you want.

@algesten
Copy link
Owner

Closing since we are moving to 3.x. The HTTP crate API will have the functions required.

@algesten algesten closed this Aug 13, 2024
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.

Getting the list of all the headers and their content
3 participants