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

Merged
merged 4 commits into from
Sep 9, 2024

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 semver/patch No public API change. 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 swiftlang/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?

@FranzBusch
Copy link
Member

I am going to close this PR in favour of #2864 which has the same effect

@FranzBusch FranzBusch closed this Sep 4, 2024
@dnadoba
Copy link
Member Author

dnadoba commented Sep 4, 2024

It doesn’t. It now has two separate implementations but the point of this PR is that they should be identical as the normal description already is appropriate for structural display . With the linked PR they can now diverge from each other which isn’t a good. Ideally we would remove the conformance but that is source breaking.

@dnadoba dnadoba reopened this Sep 4, 2024
@FranzBusch FranzBusch enabled auto-merge (squash) September 5, 2024 07:44
@@ -971,18 +971,8 @@ extension ByteBuffer: CustomStringConvertible, CustomDebugStringConvertible {
"[\(self.hexDump(format: .compact(maxBytes: 64)))](\(self.readableBytes) bytes)"
}

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

Choose a reason for hiding this comment

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

@dnadoba why delete the comment?

@FranzBusch FranzBusch merged commit d2713ab into apple:main Sep 9, 2024
28 of 29 checks passed
ali-ahsan-ali pushed a commit to ali-ahsan-ali/swift-nio that referenced this pull request Sep 15, 2024
…pple#2495)

### 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 `Array`s 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 apple#2475 landed e.g.
```swift
extension ByteBuffer {
    struct PrintFormat {
        static let hex: Self
        static let decimal: Self
        ...
    }
    func descriptionWithContents(format: PrintFormat = .hex, maxBytes: Int = 1024) {
        ...
    }
}
```

Co-authored-by: Franz Busch <[email protected]>
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Sep 25, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [apple/swift-nio](https://redirect.github.com/apple/swift-nio) | minor
| `2.72.0` -> `2.73.0` |

---

### Release Notes

<details>
<summary>apple/swift-nio (apple/swift-nio)</summary>

###
[`v2.73.0`](https://redirect.github.com/apple/swift-nio/releases/tag/2.73.0)

[Compare
Source](https://redirect.github.com/apple/swift-nio/compare/2.72.0...2.73.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### SemVer Minor

- Make `ByteBuffer`'s description more useful by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2864](https://redirect.github.com/apple/swift-nio/pull/2864)
- Expose `UDP_MAX_SEGMENTS` via System by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2891](https://redirect.github.com/apple/swift-nio/pull/2891)
- Add new `ChannelOption` to get the amount of buffered outbound data in
the Channel by
[@&#8203;johnnzhou](https://redirect.github.com/johnnzhou) in
[https://github.com/apple/swift-nio/pull/2849](https://redirect.github.com/apple/swift-nio/pull/2849)
- Add an `AcceptBackoffHandler` to the async server bootstraps by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2782](https://redirect.github.com/apple/swift-nio/pull/2782)

##### SemVer Patch

- Adding a nicer description for `WebSocketFrame` by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2862](https://redirect.github.com/apple/swift-nio/pull/2862)
- Improving `description` and adding `debugDescription` to `NIOAny` by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2866](https://redirect.github.com/apple/swift-nio/pull/2866)
- Make FileChunk sendable by
[@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali) in
[https://github.com/apple/swift-nio/pull/2871](https://redirect.github.com/apple/swift-nio/pull/2871)
- Make `ByteBuffer.debugDescription` suitable for structural display by
[@&#8203;dnadoba](https://redirect.github.com/dnadoba) in
[https://github.com/apple/swift-nio/pull/2495](https://redirect.github.com/apple/swift-nio/pull/2495)
- Add support for WASILibc by
[@&#8203;MaxDesiatov](https://redirect.github.com/MaxDesiatov) in
[https://github.com/apple/swift-nio/pull/2671](https://redirect.github.com/apple/swift-nio/pull/2671)
- `NIOSingleStepByteToMessageDecoder` reentrancy safety by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2881](https://redirect.github.com/apple/swift-nio/pull/2881)
- Adopt `NIOThrowingAsyncSequenceProducer` by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2879](https://redirect.github.com/apple/swift-nio/pull/2879)
- Clamp buffer to maximum upon large write operation by
[@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali) in
[https://github.com/apple/swift-nio/pull/2745](https://redirect.github.com/apple/swift-nio/pull/2745)
- Revert "Adopt `NIOThrowingAsyncSequenceProducer`
([#&#8203;2879](https://redirect.github.com/apple/swift-nio/issues/2879))"
by [@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2892](https://redirect.github.com/apple/swift-nio/pull/2892)
- Add concrete description for `EmbeddedEventLoop` by
[@&#8203;aryan-25](https://redirect.github.com/aryan-25) in
[https://github.com/apple/swift-nio/pull/2890](https://redirect.github.com/apple/swift-nio/pull/2890)
- Conditionally include linux/udp.h by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2894](https://redirect.github.com/apple/swift-nio/pull/2894)
- Work around a type checking error when using the Static Linux SDK by
[@&#8203;euanh](https://redirect.github.com/euanh) in
[https://github.com/apple/swift-nio/pull/2898](https://redirect.github.com/apple/swift-nio/pull/2898)

##### Other Changes

- \[CI] Run tests on push to main by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2868](https://redirect.github.com/apple/swift-nio/pull/2868)
- \[CI] License header support `.in` and `.cmake` files by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2870](https://redirect.github.com/apple/swift-nio/pull/2870)
- Include nanoseconds in assertion of timestamp for NIOFileSystem tests
by [@&#8203;gjcairo](https://redirect.github.com/gjcairo) in
[https://github.com/apple/swift-nio/pull/2869](https://redirect.github.com/apple/swift-nio/pull/2869)
- Correct the link of sswg-security at SECURITY.md by
[@&#8203;lamtrinhdev](https://redirect.github.com/lamtrinhdev) in
[https://github.com/apple/swift-nio/pull/2872](https://redirect.github.com/apple/swift-nio/pull/2872)
- Speculative fix for flakey AsyncTestingEventLoop test by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2873](https://redirect.github.com/apple/swift-nio/pull/2873)
- ci: Install shellcheck if not present in CI runner by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2882](https://redirect.github.com/apple/swift-nio/pull/2882)
- ci: Use ${GITHUB_BASE_REF} as treeish for checking API break by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2883](https://redirect.github.com/apple/swift-nio/pull/2883)
- ci: Refer to nested reusable workflows using remote variant by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2884](https://redirect.github.com/apple/swift-nio/pull/2884)
- \[CI] Fix pull request label workflow by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2885](https://redirect.github.com/apple/swift-nio/pull/2885)

#### New Contributors

- [@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali)
made their first contribution in
[https://github.com/apple/swift-nio/pull/2871](https://redirect.github.com/apple/swift-nio/pull/2871)
- [@&#8203;aryan-25](https://redirect.github.com/aryan-25) made their
first contribution in
[https://github.com/apple/swift-nio/pull/2890](https://redirect.github.com/apple/swift-nio/pull/2890)
- [@&#8203;johnnzhou](https://redirect.github.com/johnnzhou) made their
first contribution in
[https://github.com/apple/swift-nio/pull/2849](https://redirect.github.com/apple/swift-nio/pull/2849)
- [@&#8203;euanh](https://redirect.github.com/euanh) made their first
contribution in
[https://github.com/apple/swift-nio/pull/2898](https://redirect.github.com/apple/swift-nio/pull/2898)

**Full Changelog**:
apple/swift-nio@2.72.0...2.73.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC45NC4xIiwidXBkYXRlZEluVmVyIjoiMzguOTQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants