swift-nio: `ByteBuffer.set(string:)` returns an `Optional` Int

Expected behavior

ByteBuffer.set(string: String, at index: Int) -> Int

The set(string:at:) (and write(string:)) return an Optional<Int> even though they never fail (looking at the implementation). Presumably (and talking to @weissi ) this is a left-over from an old variant of the func which supported different encodings.

Also: the comment doesn’t say anything about the optionality, making it even more confusing. Does it mean no buffer space is left? Does it mean the encoding failed? Does it mean Mercury passed Venus?

Since the buffer is auto-expanding, I think this should just not fail and return a plain Int.

Actual behavior

ByteBuffer.set(string: String, at index: Int) -> Int?

SwiftNIO version/commit hash

Head of today, shouldn’t matter.

Swift & OS version (output of `swift --version && uname -a`)

Any.

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 15 (9 by maintainers)

Commits related to this issue

Improve ByteBuffer String APIs Motivation: ByteBuffer has pretty good string writing APIs, but they're imperfect. In particular, the methods in ByteBuffer-aux both return optional Int, which is stra... — committed to Lukasa/swift-nio by Lukasa 6 years ago
Improve ByteBuffer String APIs Motivation: ByteBuffer has pretty good string writing APIs, but they're imperfect. In particular, the methods in ByteBuffer-aux both return optional Int, which is stra... — committed to Lukasa/swift-nio by Lukasa 6 years ago
Improve ByteBuffer String APIs (#696) Motivation: ByteBuffer has pretty good string writing APIs, but they're imperfect. In particular, the methods in ByteBuffer-aux both return optional Int, wh... — committed to apple/swift-nio by Lukasa 6 years ago

Most upvoted comments

My feeling is that they don’t want you to overload the method name like that. The base name should denote what it does, and if it is overloading, just the type.

I’m looking primarily at the “Compensate for weak type information” of the API Design Guidelines. Look how they do addObserver(...) instead of add(observer:).

Or makeWidget(_...) not make(widget:...) in the same document.

But I might be completely wrong on this, I also walked over this naming issue a few times myself and wasn’t quite sure. Though I think your own example is actually pretty good. It should be this (I guess):

func writeString(_ s: UnsafePointer<UInt8>)
func writeString(_ s: StaticString)
func writeString(_ s: String)

What you do is you write a string. What you write can come from various value types and the type could be different. It would even cover my case:

func writeString(_ i: Integer, radix: Int = 10)

Which kinda shows the thinking.

Again, I might be really wrong on this. The only practical issue w/ the current style is that Xcode is really bad at completing overloaded base names, but that is more of an Xcode issue.

helje5 on Mar 29, 2018

@weissi If the error can be meaningful, the function should be throwing. The combination of throws and @discardableResult should give exactly what you describe: possibility to ignore the resulting Int value, but require attention to possible error.

moiseev on Mar 29, 2018