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

Fix error description forwarding #143

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Feb 24, 2025

Motivation

Fixes apple/swift-openapi-generator#730.

We were not correctly keeping CustomStringConvertible and LocalizedError conformances separate for wrapper errors like ClientError and ServerError. This lead to some user-thrown errors (in handlers, transports, and middlewares) to print less information than the error was actually providing (using a different method).

Modifications

Properly untangle the two printing codepaths, and only call localizedDescription from the wrapper error's errorDescription.

Also made the localizedDescription strings a bit more user-friendly and less detailed, as in some apps these errors might get directly rendered by a UI component that calls localizedDescription.

Result

Error logging should now match adopter expectations.

Test Plan

Added unit tests for {Client,Server}Error printing methods.

Verified

This commit was signed with the committer’s verified signature.
czechboy0 Honza Dvorsky
@czechboy0
Copy link
Contributor Author

cc @weissi

@czechboy0 czechboy0 mentioned this pull request Feb 24, 2025
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Feb 24, 2025
Copy link
Collaborator

@simonjbeaumont simonjbeaumont 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, thanks @czechboy0!

Took me a minute to confirm that prettyDescription didn't get in the way again, but I believe this resolves the ask and we can avoid making use of localized description in pretty description going forward too.

@simonjbeaumont simonjbeaumont merged commit acb9425 into apple:main Feb 24, 2025
24 of 25 checks passed
@czechboy0 czechboy0 deleted the hd-fix-error-descriptions branch February 24, 2025 11:31
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.

Separate error description vs localizedDescription
2 participants