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

Protobuf generated classes should extend from Google::Protobuf::AbstractMessage #1911

Merged
merged 11 commits into from
Jun 4, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented May 28, 2024

Motivation

Problem reported in Shopify Core that to_proto wasn't being recognized.

Implementation

Mix in the necessary module.
Inherit from Google::Protobuf::AbstractMessage.

Tests

Updated.

@andyw8 andyw8 added the bugfix label May 28, 2024
@andyw8 andyw8 force-pushed the andyw8/protobuf-messageexts branch from 494efb3 to e8bf434 Compare May 28, 2024 19:31
@andyw8 andyw8 marked this pull request as ready for review June 3, 2024 21:27
@andyw8 andyw8 requested a review from a team as a code owner June 3, 2024 21:27
@@ -86,6 +88,7 @@ def decorate
elsif constant == Google::Protobuf::Map
create_type_members(klass, "Key", "Value")
else
klass.create_include("Google::Protobuf::MessageExts")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we should instead make these classes subclass from Google::Protobuf::AbstractMessage instead, which pulls in this include as well: https://github.com/protocolbuffers/protobuf/blob/5d40c4f43409bbcdf7ab2d24cbb4b1c363cfa7a0/ruby/lib/google/protobuf/message_exts.rb#L29

It is a private constant, but since it will only be inside our generated RBI files, that shouldn't be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

block: T.nilable(T.proc.params(scope: Scope).void),
).returns(Scope)
end
def create_path(constant, superclass_name = nil, &block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change doesn't make a lot of sense. We would allow people to pass a superclass_name for a module constant and just silently drop it?

Instead you could do something like this:

root.create_path(constant) do |klass|
  raise unless klass.is_a?(RBI::Class)

  klass.superclass_name = "Google::Protobuf::AbstractMessage"
  # ...

lib/tapioca/dsl/compilers/protobuf.rb Outdated Show resolved Hide resolved
manual/compiler_protobuf.md Outdated Show resolved Hide resolved
@andyw8 andyw8 changed the title Protobuf include Google::Protobuf::MessageExts Protobuf extend from Google::Protobuf::AbstractMessage Jun 4, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

Updated with @Morriar's suggestion, but looking into the 1 failing test.

@andyw8 andyw8 changed the title Protobuf extend from Google::Protobuf::AbstractMessage Protobuf generated class should extend from Google::Protobuf::AbstractMessage Jun 4, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

Passing on Core.

@andyw8 andyw8 changed the title Protobuf generated class should extend from Google::Protobuf::AbstractMessage Protobuf generated classes should extend from Google::Protobuf::AbstractMessage Jun 4, 2024
@andyw8 andyw8 merged commit 95e60bc into main Jun 4, 2024
30 checks passed
@andyw8 andyw8 deleted the andyw8/protobuf-messageexts branch June 4, 2024 18:50
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 19, 2024

(merging this commit history was unintentional, I was used to the auto-squash in other repos).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants