swift-nio icon indicating copy to clipboard operation
swift-nio copied to clipboard

Make `ByteBuffer.debugDescription` suitable for structural display

Open dnadoba opened this issue 2 years ago • 5 comments

Motivation

CustomDebugStringConvertible /debugDescription name and documentation is confusing and should be changed but it is today expected to be suitable to be used as part of a structured display e.g. to be printed as part of an Arrays description, a struct or enum with associated values. Therefore it should have no unpaired parentheses, no unescaped quotes, no top-level commas and no new lines.

Modifications

let debugDescription simply contain the same contents as description. We can't remove the property or the conformance without breaking API.

Results

ByteBuffer has a proper string representation suitable for being displayed in an Array, as the property of a struct or an associated value of an enum.

We can add a new property/method once #2475 landed e.g.

extension ByteBuffer {
    struct PrintFormat {
        static let hex: Self
        static let decimal: Self
        ...
    }
    func descriptionWithContents(format: PrintFormat = .hex, maxBytes: Int = 1024) {
        ...
    }
}

dnadoba avatar Aug 07 '23 13:08 dnadoba

Indeed and I was also quite confused about it. However, debugDescription is almost always used except if you convert a ByteBuffer to a String directly. I have opened https://github.com/apple/swift/issues/67541 and subsequently learned from @lorentey that essentially a type should only conform to CustomDebugStringConvertible if it has a description that is not suitable to be used in a structural display, i.e. as part of a comma separate list in the description of an Array. Otherwise they should contain the same information.

dnadoba avatar Aug 07 '23 13:08 dnadoba

Indeed and I was also quite confused about it. However, debugDescription is almost always used except if you convert a ByteBuffer to a String directly. I have opened apple/swift#67541 and subsequently learned from @lorentey that essentially a type should only conform to CustomDebugStringConvertible if it has a description that is not suitable to be used in a structural display, i.e. as part of a comma separate list in the description of an Array. Otherwise they should contain the same information.

Ah yes, this rings a bell. The mistake was to initially add the DebugStringConvertible conformance and now we can't repair it...

Okay, then this PR should change to add back the actually useful debugDescription under a new name

weissi avatar Aug 07 '23 14:08 weissi

Yes, we can do that as a follow up as described in the PR description once https://github.com/apple/swift-nio/pull/2475 landed. This will be an API design question on its own.

dnadoba avatar Aug 07 '23 14:08 dnadoba

Note: my recommendation that the feature that distinguishes debugDescription from description is that the former's result must not cause confusion when embedded in structured syntax is not yet official -- however, it is the only way I can make practical sense of things given the stdlib's usages of this property, which are now sort of etched in stone. 😕

(This comes from the investigation I did last month for https://github.com/apple/swift-collections/issues/301.)

FWIW, I usually call the "print everything during manual debugging" method _dump().

lorentey avatar Aug 08 '23 22:08 lorentey

Yes, we can do that as a follow up as described in the PR description once https://github.com/apple/swift-nio/pull/2475 landed. This will be an API design question on its own.

I would be happy to help if I can. Do you think we should consider adding the debug description (under a new name?) in this PR, or later as a follow-up?

I remember @weissi mentioned that we might rewrite _storage, so using _storage._dumpBytes might be not such a great idea — one approach would be to wrap hexDump() with some additional internal information (reader / writer indices, capacity, etc).

Existing HexDumpFormats only cover plain and detailed hex, which I think are useful for this too, but if we want decimal format of bytes dump — we can add plain decimal option as well?

natikgadzhi avatar Sep 09 '23 04:09 natikgadzhi