Make Body.data generic #534
Conversation
|
Sadly this is a source breaking change because we can create an unapplied reference to this method e.g.: let makeBodyFromData = HTTPClient.Body.dataAfter your change this no longer compiles with the error message "Generic parameter 'Bytes' could not be inferred". What we can do instead is leaving the old method signature as is and adding your new method as an additional overload. |
karwa
force-pushed
the
this-is-a-generic-branch-name
branch
2 times, most recently
from
ed5ae39
to
09394d9
Compare
|
@dnadoba Okay, I've added the I've also added a test to make double-sure that unapplied references to |
| /// - parameters: | ||
| /// - bytes: Body `Data` representation. | ||
| public static func data(_ bytes: Data) -> HTTPClient.Body { | ||
| func forwardToGenericImpl<C>(_ value: C) -> HTTPClient.Body where C: RandomAccessCollection, C.Element == UInt8 { |
There was a problem hiding this comment.
@Lukasa do you know if the compiler will specialise this local function?
There was a problem hiding this comment.
It appears to: https://swift.godbolt.org/z/WKeeG5r7v
Note that both test and doSomething just compare to 99, without calling the generic function. This function should be equivalent to the concrete version of doSomething.
There was a problem hiding this comment.
As @karwa says, yes, the compiler should absolutely specialise this function. However, it's a pretty ugly thunk to have to have, so I'd like to ask whether we can differentiate between this method and the other method called data by renaming the other one. That way we don't need to write this thunk out explicitly.
There was a problem hiding this comment.
Hmm, well the async API seems to use bytes for this. So we could use that.
I went with data to try and remove the Foundation.Data dependency whilst still supporting Data. But if we have to add the extension for source compatibility, that reasoning isn't quite as strong. Then again, there is value in having the same thing be spelled the same way, and it's the API we have, so... ⚖️ . The thunk is not going to be winning any beauty awards, for sure, but is it less ugly than having two spellings for essentially the same operation? I'm not sure.
If you really think it's worth it, I could rename it to bytes.
There was a problem hiding this comment.
In general I'm disinclined to rely on type-based method overloads. We've had a bunch of problems with this in the past where having methods whose name is the same that differ only based on type leans excessively heavily on the type checker. If these differed only on return value I'd outright refuse to accept them having the same name: differing based on argument type is less bad, but in this instance I think I'd still rather use a different name. There's existing prior art in NIO for using the word data in method names to mean "accepts/returns Foundation.Data" and bytes to mean "accepts/returns something else", so I think we're in good company here.
There was a problem hiding this comment.
Oh, I wasn't aware that pattern was used elsewhere in NIO. I've changed it to .bytes now.
karwa
force-pushed
the
this-is-a-generic-branch-name
branch
from
09394d9
to
afaa340
Compare
| /// - parameters: | ||
| /// - bytes: Body `Data` representation. | ||
| public static func data(_ data: Data) -> HTTPClient.Body { | ||
| return bytes(data) |
There was a problem hiding this comment.
Yeah, I know this is a ridiculous nit, but:
| return bytes(data) | |
| return self.bytes(data) |
karwa
force-pushed
the
this-is-a-generic-branch-name
branch
from
afaa340
to
cbb8fa8
Compare
fabianfett
added
the
🔼 needs-minor-version-bump
I don't think there is a need for this specific API to use Foundation.Data -- we can broaden it to all RandomAccessCollections . RAC seems like the most appropriate constraint given the up-front
countcalculation.This removes a Foundation dependency, and should be source-compatible.