Skip to content

Revert "Revert "[objc] GRPCMetadataDictionary convenient typedef""#27882

Merged
dennycd merged 1 commit intomasterfrom
revert-27877-revert-27845-v2_api_update
Nov 3, 2021
Merged

Revert "Revert "[objc] GRPCMetadataDictionary convenient typedef""#27882
dennycd merged 1 commit intomasterfrom
revert-27877-revert-27845-v2_api_update

Conversation

@dennycd
Copy link
Copy Markdown
Contributor

@dennycd dennycd commented Oct 29, 2021

Reverts #27877

@dennycd dennycd added lang/ObjC platform/iOS release notes: no Indicates if PR should not be in release notes labels Oct 29, 2021
@dennycd dennycd requested a review from sampajano November 3, 2021 13:35
@dennycd
Copy link
Copy Markdown
Contributor Author

dennycd commented Nov 3, 2021

internal TGP train check all pass, this one ready to re-land, thanks

@dennycd dennycd force-pushed the revert-27877-revert-27845-v2_api_update branch from 1901b2d to 011ce1a Compare November 3, 2021 13:41
Copy link
Copy Markdown
Contributor

@sampajano sampajano left a comment

Choose a reason for hiding this comment

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

Thanks Denny. Comments inline :)

Comment thread src/objective-c/GRPCClient/GRPCTypes.h Outdated
@end

/** gRPC metadata dictionary typedef */
typedef NSDictionary<NSString*, id<NSCopying>> GRPCMetadataDictionary;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing id -> id<NSCopying> would potentially break some 3rd party users, is that right? Is this change totally necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updating this to id to avoid breaking changes in the API.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks LGTM now! :)

@@ -279,7 +279,7 @@ NS_ASSUME_NONNULL_BEGIN
* Dictionary key is of type NSString, value should be either NSString or NSData containing binary
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider moving 2 similar comments to GRPCMetadataDictionary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks. this part is specific to initialMetadata (either NSString // NSData for the value type). GRPCMetadataDictionary are the more generic definition which also need to apply to e.g. additionalChannelArgs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ohh i see! Thanks for clarification! :)

@dennycd dennycd force-pushed the revert-27877-revert-27845-v2_api_update branch from 011ce1a to bc77067 Compare November 3, 2021 20:30
@sampajano sampajano self-requested a review November 3, 2021 20:47
@dennycd dennycd merged commit b1e6b1f into master Nov 3, 2021
@copybara-service copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Nov 4, 2021
@dennycd dennycd added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Nov 6, 2021
@stanley-cheung stanley-cheung deleted the revert-27877-revert-27845-v2_api_update branch November 16, 2021 18:58
@dennycd dennycd added release notes: no Indicates if PR should not be in release notes and removed release notes: yes Indicates if PR needs to be in release notes labels Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

imported Specifies if the PR has been imported to the internal repository lang/ObjC platform/iOS release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants