-
Notifications
You must be signed in to change notification settings - Fork 365
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
cURL examples have host and cookies by default #42
Comments
I can explain why the Host and Cookie headers are included, but I am now sure how this issue should be resolved. cURL examples are generated based on the the request that was actually sent to the API when the test ran. I believe that the (default) RackTestClient will add those two headers to every request. One possible solution would be to use Rack::MockRequest instead of Rack::Test::Session. I believe that Rack::MockRequest will not alter the outgoing request in any way, but I haven't checked. We could also try to configure Rack::Test::Session to prevent this behavior. (Do Rack apps needs these headers?) We could also exclude those 2 headers explicitly from the cURL example output, but it would be Wrong to exclude the headers if their value were set intentionally in the spec file. I don't support this idea. |
Thank you for the feedback. If one defines an example like this: get '/orders.json' do I believe it would be more intuitive if the default headers were not used, given that the developer is already specifing the headers that are to be used. Conversely, adding a no_headers option could also drop all existing records, but that wouldn't be as intuitive. I really don't see the value of the default headers. Most end-users will simply do a 'curl url' and be done with it, I think.. |
I agree that it is confusing to include the headers when they weren't requested. It's similarly confusing when they same headers appear in every other output format, as they currently do. Eric and I agree that "hiding" the headers from the generated output is a bad idea (then your test is different from your documentation!). We need to find a way to prevent these headers from being sent in the first place. |
+1 |
Chiming in pretty late but, I'm not entirely sure how to handle this. We could potentially change this line to be: def request_headers
@request_headers
end
protected
def do_request(method, path, params, request_headers)
@request_headers = headers(method, path, params, request_headers)
# ...
end This way we ignore whatever else gets added via Rack::Test. If someone wants to specify the |
Since the output for the curl is just for a curl example, I don't see the harm in filtering out blank headers. Are there any cases where you need to include an empty header for the request to work? For me that sounds just... wrong. |
Right now, to hide these blank headers when visualizing the docs (reading from the json) is pretty easy. But to change the generated curl it requires way more effort. |
I added the ability to filter out cURL headers per this issue. I'm not entirely sold on filtering the example request in the documentation itself. I am fine with the cURL example because it's meant to be something the user can copy and paste, not necessarily documentation. I sort of think that the documentation viewer should be the one filtering out headers since it's just a Hash. The HTML generated with this plugin isn't really meant to be your final documentation since it's very plain. |
Great! That's exactly what I needed. As you said, the viewer can choose from the json array which headers to display. My problem was with the curl output that is plain text. This patch allows me to remove it from the curl generation, so now I can filter out headers for my API consistently :) Thank you! |
This shouldn't be the case, otherwise you get confusing examples such as:
curl "https://foo.com/orders.json" -X GET -H "Accept: application/json" -H "Content-Type: application/json" -H "Host: example.org" -H "Cookie: "
Those two headers really confuse people when they read through the docs. Is there any real value added in having them, as is?
The text was updated successfully, but these errors were encountered: