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

Define Ruby Struct classes under GRPC::Core:: #19931

Closed

Conversation

blowmage
Copy link
Contributor

It looks like the intention was to define these classes under GRPC::Core::, but they are currently being defined under Struct:: (which is the default for rb_struct_define).

If you want this change to be 100% backwards compatible, we can also add the following aliases:

Struct:: BatchResult  = GRPC::Core:: BatchResult
Struct:: NewServerRpc = GRPC::Core:: NewServerRpc
Struct:: Status       = GRPC::Core:: Status

These classes are currently defined under Struct::
def to_status
Struct::Status.new(code, details, metadata)
GRPC::Core::Status.new(code, details, metadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think changing the Status module will work for backwards compatibility reasons. Note that while NewServerRpc and BatchResult are relatively internal objects, Struct::Status is a part of the public surface API.

Can we solve the problem in #19893 (comment) by just moving the definition of Status out of the C extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about moving the definition and then aliasing the old name to the new name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Though this would work, it's a slight complication - what's the motivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivation is only that it looks like the intention would have been to locate these classes in the GRPC namespace instead of using the default location under Struct.

In Ruby I would expect an anonymous struct to be assigned to a constant:

GRPC::Core::Status = Struct.new(:code, :details, :metadata)

Instead of creating a named struct that uses the default location:

Struct.new('Status', :code, :details, :metadata) # Struct::Status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to put this on pause and revisit it again once #19939 has been considered.

@apolcyn
Copy link
Contributor

apolcyn commented Aug 13, 2019

Noting this is a "release blocker" to make sure that the comment in #19893 (comment) is resolved before 1.24

(this isn't a 1.23 release blocker)

@blowmage
Copy link
Contributor Author

FWIW, I think #19939 would be the release blocker, not this change.

@stale
Copy link

stale bot commented Feb 10, 2020

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

@stale stale bot closed this Feb 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants