Making ByteBuffer's description more useful #2864

supersonicbyte · 2024-09-03T13:02:16Z

Motivation:

Resolving the following issue: #2863

Modifications:

Rewrote description and debugDescription on ByteBuffer to make it more useful. Also fixed a bug in the in hexDumpCompact.

Result:

A more useful description and debugDescription.

Motivation: Resolving the following issue: apple#2863 Modifications: Rewrote `description` and `debugDescription` on `ByteBuffer` to make it more useful. Result: A more userful `description` and `debugDescription`. After your change, what will change.

weissi

LGTM thank you. I'll let the NIO team actually review this before they can merge.

weissi · 2024-09-03T14:37:24Z

Sources/NIOCore/ByteBuffer-core.swift

@@ -957,37 +957,30 @@ public struct ByteBuffer {
 }

 extension ByteBuffer: CustomStringConvertible, CustomDebugStringConvertible {
-    /// A `String` describing this `ByteBuffer`. Example:
+    /// A `String` describing this `ByteBuffer`.  For a `ByteBuffer` initialised with `hello world`


Suggested change

/// A `String` describing this `ByteBuffer`. For a `ByteBuffer` initialised with `hello world`

/// A `String` describing this `ByteBuffer` including length and the bytes it contains (partially).

///

/// For a `ByteBuffer` initialised with `hello world`

Nit: The first line in the doc comments is special, it's the summary. That summary gets displayed by itself in many scenarios so we should aim for it to make sense without the rest of the doc comment.

weissi · 2024-09-03T14:38:40Z

Tests/NIOCoreTests/NIOAnyDebugTest.swift

@@ -24,7 +24,7 @@ class NIOAnyDebugTest: XCTestCase {
        let bb = ByteBuffer(string: "byte buffer string")
        XCTAssertTrue(
            wrappedInNIOAnyBlock(bb).contains(
-                "NIOAny { ByteBuffer { readerIndex: 0, writerIndex: 18, readableBytes: 18, capacity: 32, storageCapacity: 32, slice: _ByteBufferSlice { 0..<32 }, storage: "
+                "NIOAny { [627974652062756666657220737472696e67](18 bytes) }"


Nit: I almost think with this change we should change NIOAny's description to include ByteBuffer: when it's the case .byteBuffer. Previously that wasn't necessary because ByteBuffer.description contained the word ByteBuffer but that's now changed.

Looking at NIOAny I would improve its description altogether to include information about the wrapped type. But I would do that in a separate issue/PR.

weissi · 2024-09-03T14:39:20Z

Tests/NIOCoreTests/ByteBufferTest.swift

@@ -3602,5 +3607,12 @@ extension ByteBufferTest {
            XCTAssertEqual(error as? Base64Error, .invalidCharacter)
        }
    }
-
+
+    func testByteBufferDescription() {


think it would be nice to add two other test cases:

the empty ByteBuffer

one where the hex bytes get truncated (i.e. more than 128 bytes or whatever the limit is)

Thanks, added!

FranzBusch

This LGTM. @supersonicbyte please raise another PR for adapting NIOAny then

As per: apple#2864 (comment) we would like that the `description` of `NIOAny` prints the type of the underlaying value that it wraps. Changing `description` and adding a conformance to `CustomDebugStringConvertible`. A nicer `description` and `debugDescription` for `NIOAny`.

) Improving `description` and adding `debugDescription` to `NIOAny` ### Motivation As per: #2864 (comment) we would like that the `description` of `NIOAny` prints the type of the underlaying value that it wraps. ### Modification: Changing `description` and adding a conformance to `CustomDebugStringConvertible`. ### Result: A nicer `description` and `debugDescription` for `NIOAny`. Co-authored-by: Franz Busch <[email protected]>

…ple#2866) Improving `description` and adding `debugDescription` to `NIOAny` ### Motivation As per: apple#2864 (comment) we would like that the `description` of `NIOAny` prints the type of the underlaying value that it wraps. ### Modification: Changing `description` and adding a conformance to `CustomDebugStringConvertible`. ### Result: A nicer `description` and `debugDescription` for `NIOAny`. Co-authored-by: Franz Busch <[email protected]>

supersonicbyte mentioned this pull request Sep 3, 2024

Make ByteBuffer.description useful and pretty #2863

Open

weissi approved these changes Sep 3, 2024

View reviewed changes

weissi added semver/patch No public API change. semver/minor Adds new public API. and removed semver/patch No public API change. labels Sep 3, 2024

weissi reviewed Sep 3, 2024

View reviewed changes

supersonicbyte added 2 commits September 4, 2024 08:45

Improve doc comment

961b455

Add unit tests for empty and truncated ByteBuffer description

df18d08

FranzBusch approved these changes Sep 4, 2024

View reviewed changes

Merge branch 'main' into byte-buffer-description

792c96a

FranzBusch enabled auto-merge (squash) September 4, 2024 07:52

FranzBusch mentioned this pull request Sep 4, 2024

Make ByteBuffer.debugDescription suitable for structural display #2495

Merged

FranzBusch merged commit 3615f9c into apple:main Sep 4, 2024
28 of 29 checks passed

supersonicbyte mentioned this pull request Sep 4, 2024

Improving description and adding debugDescription to NIOAny #2866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making ByteBuffer's description more useful #2864

Making ByteBuffer's description more useful #2864

supersonicbyte commented Sep 3, 2024

weissi left a comment

weissi Sep 3, 2024

weissi Sep 3, 2024

supersonicbyte Sep 4, 2024

weissi Sep 3, 2024

supersonicbyte Sep 4, 2024

FranzBusch left a comment

-    /// A `String` describing this `ByteBuffer`.  For a `ByteBuffer` initialised with `hello world`
+    /// A `String` describing this `ByteBuffer` including length and the bytes it contains (partially).
+    ///
+    /// For a `ByteBuffer` initialised with `hello world`

Making ByteBuffer's description more useful #2864

Making ByteBuffer's description more useful #2864

Conversation

supersonicbyte commented Sep 3, 2024

Motivation:

Modifications:

Result:

weissi left a comment

Choose a reason for hiding this comment

weissi Sep 3, 2024

Choose a reason for hiding this comment

weissi Sep 3, 2024

Choose a reason for hiding this comment

supersonicbyte Sep 4, 2024

Choose a reason for hiding this comment

weissi Sep 3, 2024

Choose a reason for hiding this comment

supersonicbyte Sep 4, 2024

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment