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

Headers #26

Merged
merged 7 commits into from
Sep 6, 2015
Merged

Headers #26

merged 7 commits into from
Sep 6, 2015

Conversation

ksamborski
Copy link

Hi!

I needed some headers support so I added it. Please check if it's fine or not with your idea.

Regards,
Karol

RestClient::headermap headers;
headers["If-Modified-Since"] = "Sat, 29 Oct 1994 19:43:31 GMT";
RestClient::response res = RestClient::get("http://httpbin.org/cache", headers);
EXPECT_EQ(304, res.code);
Copy link
Owner

Choose a reason for hiding this comment

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

can you change this code to run against the /headers endpoint and check the actual return body that the header was set properly? I think that makes for a much better test case and I have the hope that if postmanlabs/httpbin#248 gets fixed, we can extend the unit tests to the other methods.

@mrtazz
Copy link
Owner

mrtazz commented Sep 3, 2015

Awesome, thanks so much for this contribution! I left one comment for improving the unit test, if you could fix that, I'll merge it.

@mrtazz mrtazz added this to the v0.3.0 milestone Sep 3, 2015
@ksamborski
Copy link
Author

OK, I've fixed that. Please check it.

@mrtazz
Copy link
Owner

mrtazz commented Sep 5, 2015

@ksamborski awesome, thank you! Can you also merge the current master into your branch? The pull request seems to have a conflict right now.

@ksamborski
Copy link
Author

I've merged it. I hope it's fine now :)

@mrtazz
Copy link
Owner

mrtazz commented Sep 6, 2015

awesome! thank you!

mrtazz added a commit that referenced this pull request Sep 6, 2015
@mrtazz mrtazz merged commit 85d8d5c into mrtazz:master Sep 6, 2015
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.

2 participants