Skip to content

Prepare async/await API for public release #531

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
merged 12 commits into from
Jan 14, 2022

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Dec 17, 2021

  • add nescary @inlinable/@usableFromInlinable for generic HTTPClientReuqest.Body builder metods.
  • rename HTTPClientReuqest.Body.byteBuffer(_:) to HTTPClientReuqest.Body.bytes(_:) to be more in line with the other overloads
  • add a new type called HTTPClientReuqest.Body.Length instead of Int? to specify the length of a body if we can't infer the length
  • move length parameter to the end

@fabianfett fabianfett added the 🆕 semver/minor Adds new public API. label Dec 20, 2021
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good

private let stream: IteratorStream

fileprivate init(stream: IteratorStream) {
self.stream = stream
}

func next() async throws -> ByteBuffer? {
public func next() async throws -> ByteBuffer? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question rather than suggestion: should this be mutating to indicate that next() consumes from the stream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense to me. @fabianfett any objections?

@@ -12,9 +12,16 @@
//
//===----------------------------------------------------------------------===//

enum RequestBodyLength: Hashable {
public struct RequestBodyLength {
Copy link
Member

Choose a reason for hiding this comment

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

We should not add this type without prefix to the top namespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I have reverted the change and added a new public type HTTPClientRequest.Body.Length which is baked by RequestBodyLength and used as the public type instead.

@dnadoba dnadoba force-pushed the dn-async-await-public branch 2 times, most recently from 509784b to 2702cb0 Compare December 21, 2021 08:29
extension HTTPClientRequest.Body {
public struct Length {
/// size of the request body is not known before starting the request
public static let dynamic: Self = .init(storage: .dynamic)
Copy link
Member

Choose a reason for hiding this comment

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

Is dynamic the right name here? What about unknown?
I like the storage workaround btw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

works for me

Copy link
Member

Choose a reason for hiding this comment

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

btw. fixed is also weird. I guess known would be more appropriate. Also I think this type should be ExpressibleByIntegerLiteral.

@dnadoba
Copy link
Collaborator Author

dnadoba commented Dec 21, 2021

@swift-server-bot test this please

@dnadoba dnadoba changed the title Make all required types for async/await public Prepare async/await API for public release Dec 22, 2021
@dnadoba dnadoba requested a review from fabianfett December 22, 2021 12:47
@dnadoba dnadoba force-pushed the dn-async-await-public branch from f8bab65 to 10b8bd2 Compare January 14, 2022 10:48
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM!

@dnadoba dnadoba merged commit c2805df into swift-server:main Jan 14, 2022
@dnadoba dnadoba deleted the dn-async-await-public branch January 14, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants