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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 3, 2021
Merged

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

merged 1 commit into from
Nov 3, 2021

Conversation

dennycd
Copy link
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
Copy link
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
sampajano
sampajano reviewed Nov 3, 2021
View reviewed changes
Copy link
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 :)

src/objective-c/GRPCClient/GRPCTypes.h Outdated
@@ -192,3 +192,6 @@ typedef char* _Nonnull GRPCTransportID;
- (void)getTokenWithHandler:(void (^_Nonnull)(NSString* _Nullable token))handler;

@end

/** gRPC metadata dictionary typedef */
typedef NSDictionary<NSString*, id<NSCopying>> GRPCMetadataDictionary;
Copy link
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
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
Contributor

Choose a reason for hiding this comment

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

Thanks LGTM now! :)

src/objective-c/GRPCClient/GRPCCallOptions.h
@@ -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
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
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
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
Reviewers

@sampajano sampajano sampajano approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dennycd @sampajano