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

HTTP 404 in haskey, and json #47

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

nlw0
Copy link
Collaborator

@nlw0 nlw0 commented Jul 8, 2022

A couple of bugfixes. First of all I've had situations where Content-Type header contained an encoding specification. This ignores it. A couple of other changes in the first patch also seemed necessary. (I was working on top of an older version, not master).

The second patch is because the HTTP request raises an exception if the result is 404, but this is the natural result from non-existing files. The result was that simply calling haskey throws and exception when the result should be false. I'm not sure how to prevent HTTP.jl to simple return a 404 result without throwing an exception. This patches make things work, ignoring the HTTP.ExceptionRequest.StatusError exception.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022

Codecov Report

Merging #47 (3991004) into master (53474f7) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master     #47      +/-   ##
=========================================
- Coverage    0.68%   0.68%   -0.01%     
=========================================
  Files          10      10              
  Lines         436     440       +4     
=========================================
  Hits            3       3              
- Misses        433     437       +4     
Impacted Files Coverage Δ
src/collection.jl 0.00% <0.00%> (ø)

@nlw0 nlw0 force-pushed the nic/http404-and-json branch from 6bbc07b to 3991004 Compare July 21, 2022 12:03
@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 21, 2022

trying to merge master on these old prs... it would be great to merge #44 and #47, I cannot use this library without these patches

@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 22, 2022

Also this fixes #45

@nlw0
Copy link
Collaborator Author

nlw0 commented Nov 30, 2022

I have a couple of PRs that I cannot live without, would be great to get this merged. Who can review this for me? @quinnj @mattBrzezinski

@mattBrzezinski
Copy link
Member

Sorry, been caught up with other things to look here. Can you modify the Project.toml to give HTTP a compat entry, and to bump the project version appropriately?

@nlw0
Copy link
Collaborator Author

nlw0 commented Dec 1, 2022

Sorry, been caught up with other things to look here. Can you modify the Project.toml to give HTTP a compat entry, and to bump the project version appropriately?

Isn't that what @quinnj did in the latest patches from July 4 🇺🇸 🎆 🌭 ? Anything still missing?

@mattBrzezinski
Copy link
Member

Sorry, been caught up with other things to look here. Can you modify the Project.toml to give HTTP a compat entry, and to bump the project version appropriately?

Isn't that what @quinnj did in the latest patches from July 4 🇺🇸 🎆 🌭 ? Anything still missing?

Ah sorry, this package is really weird... Just needs to be marked as 0.10.0 and it's good to go, I'll force merge through to avoid the formatting stuff. Might even just remove that myself.

@nlw0 nlw0 mentioned this pull request Dec 5, 2022
@mattBrzezinski mattBrzezinski merged commit b375ea5 into JuliaCloud:master Dec 7, 2022
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.

3 participants