Skip to content

Conversation

cpriebe
Copy link
Contributor

@cpriebe cpriebe commented Apr 21, 2022

This adds a shortDescription property to HTTPClientError which provides a short
description of the error without associated values.

This is useful for e.g. metric labels which should not contain dynamic, potentially unbounded, associated values.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

6 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@fabianfett fabianfett self-requested a review April 21, 2022 13:08
@@ -949,6 +949,73 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
return "HTTPClientError.\(String(describing: self.code))"
}

public var shortDescription: String {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a comment that describes, when users should use shortDescription and what they need to take into consideration when adding another case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added now. Happy to adjust the comments of course!

@cpriebe cpriebe force-pushed the cp/httpclienterror-shortdescription branch from 3a08807 to 0becaf9 Compare April 21, 2022 14:05
@cpriebe cpriebe requested a review from fabianfett April 21, 2022 14:06
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

One small change :) Since this is a public API change, let's get @Lukasa on here as well.

@@ -949,6 +949,76 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
return "HTTPClientError.\(String(describing: self.code))"
}

/// Short description of the error that can be used in case a bounded set of error descriptions is expected, e.g. to
/// include in metric labels. The description does not contain associated values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// include in metric labels. The description does not contain associated values.
/// include in metric labels. For this reason the description must not contain associated values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Do you still want to keep the in-code comment two lines down, saying essentially the same?

@fabianfett fabianfett requested a review from Lukasa April 21, 2022 14:37
This adds a shortDescription property to HTTPClientError which provides a short
description of the error without associated values.
@cpriebe cpriebe force-pushed the cp/httpclienterror-shortdescription branch from 0becaf9 to 5b0496f Compare April 21, 2022 14:54
@fabianfett
Copy link
Member

@swift-server-bot test this please

@Lukasa
Copy link
Collaborator

Lukasa commented Apr 21, 2022

@swift-server-bot add to allowlist

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! Let's wait for @Lukasa thumb though.

@fabianfett fabianfett added the 🆕 semver/minor Adds new public API. label Apr 21, 2022
@Lukasa Lukasa merged commit 89b0da2 into swift-server:main Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants