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 Metadata's description for prettier printing #2185

Merged
merged 9 commits into from
Jan 31, 2025

Conversation

gjcairo
Copy link
Collaborator

@gjcairo gjcairo commented Jan 28, 2025

This PR changes Metadata's description to include quotes around string values.

From this:

[("some-key", some-value)]

to this:

[("some-key", "some-value")]

@gjcairo gjcairo added the semver/none No version bump required. label Jan 28, 2025
@gjcairo gjcairo requested a review from glbrntt January 28, 2025 22:25
@@ -494,15 +494,15 @@ extension Metadata.Value: ExpressibleByArrayLiteral {

extension Metadata: CustomStringConvertible {
public var description: String {
String(describing: self.map({ ($0.key, $0.value) }))
String(describing: self.map({ ($0.key.description, $0.value.description) }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test to verify the description is as we'd expect for a single Metadata.Value and for a Metadata?

@glbrntt glbrntt added 🔨 semver/patch No public API change. and removed semver/none No version bump required. labels Jan 29, 2025
@gjcairo gjcairo requested a review from glbrntt January 29, 2025 14:24
Sources/GRPCCore/Metadata.swift Outdated Show resolved Hide resolved
Sources/GRPCCore/Metadata.swift Outdated Show resolved Hide resolved
@glbrntt glbrntt added the do-not-merge This change shouldn't be merged. label Jan 29, 2025
@gjcairo gjcairo requested a review from glbrntt January 30, 2025 15:04
Copy link
Collaborator

@glbrntt glbrntt 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 Gus!

@glbrntt glbrntt removed the do-not-merge This change shouldn't be merged. label Jan 31, 2025
@glbrntt glbrntt enabled auto-merge (squash) January 31, 2025 15:21
@glbrntt glbrntt merged commit eea6b49 into grpc:main Jan 31, 2025
27 of 29 checks passed
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.

2 participants