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

Adding a nicer description for WebSocketFrame. #2862

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

supersonicbyte
Copy link
Contributor

@supersonicbyte supersonicbyte commented Sep 3, 2024

Motivation:

Resolving the following issue: #2828

Modifications:

Making WebSocketFrame conform to CustomStringConvertible.

Result:

A nicer description for WebSocketFrame.

Motivation:

Resolving the following issue: apple#2828

Modifications:

Making `WebSocketFrame` conform to `CustomStringConvertible`.

Result:

A nicer description for `WebSocketFrame`.
@weissi
Copy link
Member

weissi commented Sep 3, 2024

Thank you so much! @Lukasa to take a look. LGTM

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.

Looks good to me, thank you! Could do with a tiny unit test though

@supersonicbyte
Copy link
Contributor Author

Looks good to me, thank you! Could do with a tiny unit test though

Ofcourse, should I make a new test file or put it in an existing one? Thanks for the review.

@weissi
Copy link
Member

weissi commented Sep 3, 2024

Looks good to me, thank you! Could do with a tiny unit test though

Ofcourse, should I make a new test file or put it in an existing one? Thanks for the review.

Completely up to you. In case you're interested what I would do: Given that it's such a tiny thing I'd reuse an existing file because that's slightly less work :)

@supersonicbyte
Copy link
Contributor Author

Looks good to me, thank you! Could do with a tiny unit test though

Ofcourse, should I make a new test file or put it in an existing one? Thanks for the review.

Completely up to you. In case you're interested what I would do: Given that it's such a tiny thing I'd reuse an existing file because that's slightly less work :)

Added! Thanks.

/// The format of the description is not API.
///
/// - returns: A description of this `WebSocketFrame`.
public var description: String {
Copy link
Member

Choose a reason for hiding this comment

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

Can we please avoid adding a multiline string as a description here. description is often used in other interpolated strings such as log messages and adding line breaks can often break those formats. Instead can we add a single line for this.

Copy link
Member

Choose a reason for hiding this comment

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

good point, we should add \s to the end of the line (such that the result is just one line). I missed that currently we'll actually print multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FranzBusch very good point, thanks! I will fix it.

Copy link
Member

Choose a reason for hiding this comment

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

This is still a multiline string from what I can see. Can you use " instead of """ unless I am missing something here

Copy link
Member

Choose a reason for hiding this comment

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

@FranzBusch no it's not, it has \s everywhere but the last line.

Copy link
Member

Choose a reason for hiding this comment

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

  1> print(""" 
  2.     hello \ 
  3.     my \ 
  4.     beautiful \ 
  5.     world 
  6.     """)
hello my beautiful world

/// - returns: A description of this `WebSocketFrame`.
public var description: String {
"""
WebSocketFrame {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Can we use ( instead of { to align it with other types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofcourse!

@supersonicbyte
Copy link
Contributor Author

Updated. @FranzBusch please take a look when you find the time. Also I noticed that ByteBuffer's description uses curly braces {} so we might wanna convert those to () so we are unified with other types. Thanks.

@FranzBusch
Copy link
Member

Updated. @FranzBusch please take a look when you find the time. Also I noticed that ByteBuffer's description uses curly braces {} so we might wanna convert those to () so we are unified with other types. Thanks.

Actually thinking about this again. We should probably drop the WebsocketFrame altogether for description. Normally types don't print their type name in description. The only difference then between description and debugDescription is wrapping it in () like String works e.g.

print(frame.description) // produces masKey: foo, fin: true
print(frame.debugDescription) // produces (masKey: foo, fin: true)

@dnadoba
Copy link
Member

dnadoba commented Sep 3, 2024

Normally types don't print their type name in description.

By default it does e.g.:

struct WebSocketFrame {}
print(WebSocketFrame()) // WebSocketFrame()
debugPrint(WebSocketFrame()) // PlaygroundExecutable.WebSocketFrame()

With that being said, I'm not opposed to remove the name but we would diverge from the default.

@supersonicbyte
Copy link
Contributor Author

I dropped the names as @FranzBusch suggested. If needed otherwise I can add them back.

public var debugDescription: String {
"""
( \
maskKey: \(String(describing: self.maskKey)), \
Copy link
Member

Choose a reason for hiding this comment

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

You can just interpolate self.description here instead of duplicating the code. Can we also add a test for the debugDescription please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done!

@FranzBusch FranzBusch enabled auto-merge (squash) September 4, 2024 10:52
@FranzBusch FranzBusch merged commit 74f7b1e into apple:main Sep 4, 2024
28 of 29 checks passed
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.

4 participants