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

Make http -f display the request headers. Closes #9912 #10022

Merged
merged 4 commits into from Aug 17, 2023

Conversation

ineu
Copy link
Contributor

@ineu ineu commented Aug 16, 2023

Description

As described in #9912, the http command could display the request headers with the --full flag, which could help in debugging the requests. This PR adds such functionality.

User-Facing Changes

If http get or other http command which supports the --full flag is invoked with the flag, it used to display the headers key which contained an table of response headers. Now this key contains two nested keys: response and request, each of them being a table of the response and request headers accordingly.

image

@fdncred
Copy link
Collaborator

fdncred commented Aug 16, 2023

Nice! Thanks for adding this.

What about req_headers and res_headers?

Also, is it possible to add some tests to ensure we don't break this functionality in the future?

@ineu
Copy link
Contributor Author

ineu commented Aug 16, 2023

What about req_headers and res_headers?

As a developer I'd prefer uniform names, but as a user I feel like simple headers is still better. I expect the request_headers to be used rather rarely, so a long name is fine for this key.

Also, is it possible to add some tests to ensure we don't break this functionality in the future?

I wanted to add tests, but couldn't find existing tests for the http module as an example. The problem is that this feature make sense at the very end of the request/response process, so it sounds like a case for integration testing, and I'm absolutely no Rust expert to bring the complete web integration testing solution.

I'll try to craft some unit tests though.

@fdncred
Copy link
Collaborator

fdncred commented Aug 16, 2023

there are tests for each http command. i looked at them this morning.

maybe you could make headers a list and it contains request and response?

@ineu
Copy link
Contributor Author

ineu commented Aug 16, 2023

That could be an option, I'll check how it feels

@sholderbach sholderbach added networking All about our `http` and `url` commands and everything going accross the network. pr:release-note-mention Addition/Improvement to be mentioned in the release notes labels Aug 16, 2023
@amtoine amtoine linked an issue Aug 16, 2023 that may be closed by this pull request
@ineu
Copy link
Contributor Author

ineu commented Aug 17, 2023

Not bad, | get headers.request feels quite convenient.

@ineu
Copy link
Contributor Author

ineu commented Aug 17, 2023

The test is added

@ineu
Copy link
Contributor Author

ineu commented Aug 17, 2023

Updated the PR description and screenshot according to the current format

@fdncred
Copy link
Collaborator

fdncred commented Aug 17, 2023

Looks great!

@fdncred fdncred merged commit ec5b9b9 into nushell:main Aug 17, 2023
19 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Aug 17, 2023

Thanks for putting this together. I tested a few things and it seems to work great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking All about our `http` and `url` commands and everything going accross the network. pr:release-note-mention Addition/Improvement to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "http" commands verbose
3 participants