Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Convert writeXXXX() calls to write(xxxx:) #45

Closed
wants to merge 1 commit into from

Conversation

seabaylea
Copy link
Member

As proposed by Georgios Moschovitis on the mailing list, this converts the HTTPResponseWriter.writeXXXX() functions to HTTPResponseWriter.write(xxxx:).

This gives an API surface of:

// Headers
write(headers: HTTPHeaders, status: HTTPResponseStatus, completion: @escaping (Result) -> Void)
write(headers: HTTPHeaders, status: HTTPResponseStatus)
write(status: HTTPResponseStatus)

// Body
write(body: UnsafeHTTPResponseBody, completion: @escaping (Result) -> Void)
write(body: UnsafeHTTPResponseBody) 

// Trailers
write(trailers: HTTPHeaders, completion: @escaping (Result) -> Void)
write(trailers: HTTPHeaders)

The area worth discussing is the following function:

write(status: HTTPResponseStatus)

This is a convenience wrapper for writing headers, so a side effect is that it may not be obvious that it results in the writing of the headers to the socket - i.e. a subsequent call to write(headers:) isn't possible.

@helje5
Copy link
Contributor

helje5 commented Sep 11, 2017

I didn't follow the discussion but isn't that against:

https://swift.org/documentation/api-design-guidelines/#argument-labels

? It is tempting but addSubview is AFAIK preferred over add(subview:) etc.

@ianpartridge
Copy link
Contributor

Has consideration been given to replacing

write(body: UnsafeHTTPResponseBody, completion: @escaping (Result) -> Void)
write(body: UnsafeHTTPResponseBody)

with a single API?

write(body: UnsafeHTTPResponseBody, completion: @escaping ((Result) -> Void)? = nil)

In other words, make the completion optional and only call it if present?

@helje5
Copy link
Contributor

helje5 commented Sep 11, 2017

@ianpartridge Protocols don't support default parameters. Hence the hack w/ the extension.

@gmosx
Copy link

gmosx commented Sep 12, 2017

@helje5

Hmm, it seems you are right, the proposed API goes against the naming guidelines :(

@seabaylea
Copy link
Member Author

If we're agreed that this change doesn't make sense, is everyone happy for me to close this one?

@gmosx
Copy link

gmosx commented Sep 12, 2017

Maybe just consider renaming only:

writeBody() to write(_ body:) and leave writeHeaders() as is?

@helje5
Copy link
Contributor

helje5 commented Sep 12, 2017

That doesn't sound consistent. (It would make some sense if there would be another 'OutputStream' protocol which has such a method and is implemented, but not for such a targeted type [IMO])

@helje5
Copy link
Contributor

helje5 commented Sep 12, 2017

Something which might make sense is calling this writeChunk()? Not sure (maybe the chunking should be the responsibility of the writer? maybe not, the app layer might want to control this).

@gmosx
Copy link

gmosx commented Sep 12, 2017

@helje5

OK, I guess writeBody() is good enough...

@seabaylea
Copy link
Member Author

As the consensus appears to be to leave the API as-is, I'm going to close this.

@seabaylea seabaylea closed this Sep 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants