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

Couple of fixes for various things and making cookie tracking on by default #560

Merged
merged 4 commits into from
Jul 25, 2020

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 25, 2020

These are just a few small fixes. The biggest change is making cookies=true by default, which means we'll track cookies for users automatically. There was at least one case recently where a user was frustrated because it didn't seem like HTTP.jl could do "session management" like they were used to in python requests. We've made a couple of changes recently that make HTTP.jl act more and more like a friendlier client, so I think this makes sense.

cc-ing a couple people in case they have opinions on tracking cookies by default: @staticfloat , @StefanKarpinski , @oxinabox , @mattBrzezinski

@staticfloat
Copy link
Contributor

That's fine with me. Does HTTP also remember HTTP 301 requests?

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2020

Codecov Report

Merging #560 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   77.38%   77.43%   +0.04%     
==========================================
  Files          37       37              
  Lines        2096     2096              
==========================================
+ Hits         1622     1623       +1     
+ Misses        474      473       -1     
Impacted Files Coverage Δ
src/Handlers.jl 61.42% <100.00%> (ø)
src/cookies.jl 94.33% <100.00%> (ø)
src/StreamRequest.jl 92.45% <0.00%> (-1.89%) ⬇️
src/IOExtras.jl 64.28% <0.00%> (ø)
src/ConnectionPool.jl 78.05% <0.00%> (+0.42%) ⬆️
src/ConnectionRequest.jl 53.48% <0.00%> (+2.32%) ⬆️

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 72815f9...04ec7fb. Read the comment docs.

@quinnj
Copy link
Member Author

quinnj commented Jul 25, 2020

Ok, I reverted the cookies=true change for now, since I'd like to get more feedback and because I'm trying to get these other bugs fixed quickly.

@quinnj quinnj merged commit 8d32da0 into master Jul 25, 2020
@quinnj quinnj deleted the jq/fixes branch July 25, 2020 07:13
@oxinabox
Copy link
Member

I am a big fan of keeping managing the cookies. Cookie management by hand is just annoying.
You got a cookie, you probably wanted it and needed it for functionality.

@quinnj
Copy link
Member Author

quinnj commented Jul 25, 2020

Cool. Just fyi, I took out the cookies=true part of this PR before merging, because there were a bunch of tests that need updating and I needed the other bugfixes for my workshop this morning, but I'll clean it up and do another PR to make cookies auto-handled.

@quinnj quinnj restored the jq/fixes branch August 27, 2020 14:20
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.

None yet

4 participants