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

Separate HTTP.request from Layers.request #469

Closed
wants to merge 1 commit into from

Conversation

fredrikekre
Copy link
Member

No description provided.

@quinnj
Copy link
Member

quinnj commented Mar 19, 2020

I like the changes here; what's the status @fredrikekre ? I see it's still marked WIP?

@fredrikekre
Copy link
Member Author

I can update it if you want, wasn't sure the changes were desired and, hence, the draft.

This will break packages that insert their own layers, but maybe that is just the AWS packages? Either it needs a breaking new version here or those packages need to be fixed.

@quinnj
Copy link
Member

quinnj commented Oct 9, 2020

I'd ideally like to get #486 and this PR into a new minor release soon; @fredrikekre, are you able/willing to update here?

@fredrikekre
Copy link
Member Author

Sure.

@fredrikekre fredrikekre marked this pull request as ready for review October 10, 2020 00:51
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #469 (ce0c1d9) into master (337a372) will decrease coverage by 8.43%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
- Coverage   77.53%   69.10%   -8.44%     
==========================================
  Files          36       36              
  Lines        2333     1780     -553     
==========================================
- Hits         1809     1230     -579     
- Misses        524      550      +26     
Impacted Files Coverage Δ
src/CanonicalizeRequest.jl 0.00% <0.00%> (ø)
src/ContentTypeRequest.jl 0.00% <0.00%> (ø)
src/DebugRequest.jl 0.00% <0.00%> (-12.50%) ⬇️
src/layers.jl 86.66% <0.00%> (-13.34%) ⬇️
src/AWS4AuthRequest.jl 80.95% <100.00%> (+1.58%) ⬆️
src/BasicAuthRequest.jl 100.00% <100.00%> (ø)
src/ConnectionRequest.jl 62.16% <100.00%> (+13.72%) ⬆️
src/CookieRequest.jl 94.73% <100.00%> (+7.64%) ⬆️
src/ExceptionRequest.jl 83.33% <100.00%> (-2.39%) ⬇️
src/HTTP.jl 96.00% <100.00%> (-4.00%) ⬇️
... and 37 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 337a372...55e1328. Read the comment docs.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

This LGTM now. @mattBrzezinski @oxinabox @omus @iamed2 , will this break existing custom layers people have defined elsewhere? If so, we can rename the PR to [BREAKING] and try to announce the change.

@mattBrzezinski
Copy link
Member

This LGTM now. @mattBrzezinski @oxinabox @omus @iamed2 , will this break existing custom layers people have defined elsewhere? If so, we can rename the PR to [BREAKING] and try to announce the change.

If you remove the Layers prefix, here, and the tests fail then it'll be a breaking change.

@fredrikekre fredrikekre force-pushed the fe/sidestep-take2 branch 2 times, most recently from 84aa1c6 to ce0c1d9 Compare November 13, 2020 07:39
@fredrikekre fredrikekre added this to the 1.0 milestone May 30, 2021
@rikhuijzer rikhuijzer mentioned this pull request Aug 1, 2021
@fredrikekre fredrikekre mentioned this pull request Dec 8, 2021
12 tasks
@quinnj
Copy link
Member

quinnj commented Dec 15, 2021

@fredrikekre, are you up for updating this PR to get it merged soon for a 1.0 release?

@quinnj
Copy link
Member

quinnj commented Dec 16, 2021

Included in #789

@quinnj quinnj closed this Dec 16, 2021
@fredrikekre fredrikekre deleted the fe/sidestep-take2 branch December 16, 2021 17:27
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.

4 participants