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

Define new request class for general sanity #181

Closed
wants to merge 97 commits into from
Closed

Conversation

wsanchez
Copy link
Member

@wsanchez wsanchez commented May 24, 2017

No description provided.

@wsanchez wsanchez self-assigned this May 24, 2017
@codecov
Copy link

codecov bot commented May 24, 2017

Codecov Report

Merging #181 into master will decrease coverage by 0.28%.
The diff coverage is 96.52%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #181      +/-   ##
=========================================
- Coverage   95.89%   95.6%   -0.29%     
=========================================
  Files          12      17       +5     
  Lines        1558    2092     +534     
  Branches       86     148      +62     
=========================================
+ Hits         1494    2000     +506     
- Misses         52      72      +20     
- Partials       12      20       +8
Impacted Files Coverage Δ
src/klein/_headers.py 100% <100%> (ø)
src/klein/test/test_request.py 100% <100%> (ø)
src/klein/test/_trial.py 81.81% <81.81%> (ø)
src/klein/test/_strategies.py 89.06% <89.06%> (ø)
src/klein/_request.py 95.48% <95.48%> (ø)
src/klein/test/test_headers.py 96.98% <96.98%> (ø)
src/klein/app.py 96.37% <0%> (-2.19%) ⬇️
src/klein/test/test_resource.py 96% <0%> (-0.9%) ⬇️
src/klein/__init__.py 100% <0%> (ø) ⬆️
src/klein/test/py3_test_resource.py
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8398bf3...a80846c. Read the comment docs.

@wsanchez
Copy link
Member Author

OK so the current build failure is caused by python-hyper/hyperlink#19. Need to figure out if that's something we need to avoid, or fix elsewhere.

@wsanchez
Copy link
Member Author

@markrwilliams: Regarding bodyAsFount… the main thing that needs to happen, I think, is that IOFount needs to move from here to Tubes, and needs to be implemented correctly to allow streaming; right now it's reading the entire source in one read.

Then bodyAsFount as an addition to IRequest can build on that using logic similar to what is here. Then HTTPRequestFromIRequest would then just need to call the wrapped IRequest's implementation.

@wsanchez
Copy link
Member Author

OK, caught up with stricter URL checks, build is happy, and I think I've addressed all review items above.

Note that header name bytes should be strictly encoded as ASCII; this
interface uses ISO-8859-1 to provide interoperability with (naughty) HTTP
implementations that send non-ASCII data.
Because ISO-8859-1 is a superset if ASCII, this will still work for
Copy link
Contributor

Choose a reason for hiding this comment

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

if → of

@wsanchez
Copy link
Member Author

wsanchez commented Jul 4, 2017

At @derwolfe 's suggestion, trying to break this up a bit.

Starting with #199, which is just the new headers API.

@wsanchez
Copy link
Member Author

Withdrawing in favor of https://github.com/wsanchez/txrequest/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants