-
Notifications
You must be signed in to change notification settings - Fork 39
Add support for opting out of streaming HTTP bodies #68
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
Merged
+22
−0
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
ccc11f9
Making choice of implementation public
hactar e0b007b
Adding HTTPBodyProcessingMode
hactar 9fd926a
Making platformDefault private again
hactar a3d13fb
Making platformDefault private again
hactar d62e3cf
linting
hactar adb6c07
moving HTTPBodyProcessingMode from public enum to public struct
hactar a1b1562
removing .streamed HTTPBodyProcessingMode
hactar fb49a88
fixing testing issues
hactar 9df230c
fixing testing issues
hactar beb7d45
Merge branch 'main' into main
hactar 2f280f0
explicitly keeping init(session:)
hactar 57dfdeb
improving comments
hactar 43fd210
improving comments
hactar 5617c4d
adding .shared
hactar 9fd56c1
removing unneeded change to URLSessionTransport init
hactar 1a685fc
removing whitespace changes
hactar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding new parameters to existing functions is an API break, as caught by CI.
We'd need to preserve this symbol that calls through to the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the issue.
The new API provides
public init(session: URLSession = .shared, httpBodyProcessingMode: HTTPBodyProcessingMode = .platformDefault)
- and as there is a default parameter for httpBodyProcessingMode, doesn't Swift auto generate apublic init(session: URLSession)
for us?The following code in my app compiles fine:
So to me, there still is an (implicit)
URLSessionTransport.Configuration.init(session:)
- isn't this API breakage error a false positive?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a false positive. It's possible to refer to the function itself (e.g.
let initializer = URLSessionTransport.Configuration.init(session:)
) which won't work if the function is no longer present.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. Interesting, I wasn't aware of this limitation of default parameter values. I have added back the old single parameter init.