Skip to content
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

Make ByteBuffer.debugDescription suitable for structural display #2495

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dnadoba
Copy link
Member

@dnadoba dnadoba commented Aug 7, 2023

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 dnadoba added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Aug 7, 2023
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

This removes all the usefulness of debugDescription.

@weissi weissi self-requested a review August 7, 2023 13:40
@dnadoba
Copy link
Member Author

dnadoba commented Aug 7, 2023

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.

@weissi
Copy link
Member

weissi commented Aug 7, 2023

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

@dnadoba
Copy link
Member Author

dnadoba commented Aug 7, 2023

Yes, we can do that as a follow up as described in the PR description once #2475 landed.
This will be an API design question on its own.

@lorentey
Copy link
Member

lorentey commented Aug 8, 2023

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 apple/swift-collections#301.)

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

@natikgadzhi
Copy link
Contributor

Yes, we can do that as a follow up as described in the PR description once #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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants