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

Include the 'has_FIELD?' methods in RBIs generated from protobuf #1835

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

tgwizard
Copy link
Member

@tgwizard tgwizard commented Mar 22, 2024

Motivation

Optional fields may not be present in the serialized payload. The regular accessors will still return the default value for that field type (e.g. 0 or ""). Sometimes you want to know the difference between "default value" and "present in payload", and the has_FIELD? methods are there for that:

https://protobuf.dev/reference/ruby/ruby-generated/#checking-presence

The return type is pretty weird - it's false for when the field isn't present, and 0 for when it is.

irb(main):015> u = ShopApp::Protobuf::ServiceApi::V1::User.new
=> <ShopApp::Protobuf::ServiceApi::V1::User: id: 0, uuid: "", email_verified: false>
irb(main):016> u.email
=> ""
irb(main):017> u.has_email?
=> false
irb(main):018> u.email = "buffy@example.com"
=> "buffy@example.com"
irb(main):019> u.has_email?
=> 0
irb(main):020> u.email
=> "buffy@example.com"
irb(main):021> u.clear_email
=> nil
irb(main):022> u.has_email?
=> false

Implementation

Define the has_FIELD? method for all fields that are optional.

Tests

There are tests.

@tgwizard tgwizard requested a review from a team as a code owner March 22, 2024 10:19
@tgwizard tgwizard requested review from andyw8 and Morriar March 22, 2024 10:19
@tgwizard tgwizard added the enhancement New feature or request label Mar 22, 2024
@tgwizard
Copy link
Member Author

The failing CI is because something that seems unrelated:

cli
  gem
    generate
      it eagerly loads autoloaded constants                           PASS (3.37s)
      it uses ignores `abort` and `exit` calls inside autoloaded files PASS (5.98s)
      it must generate a gem RBI with the ones exported from the gem by default PASS (1.84s)
      it must respect include-dependencies option                     FAIL (7.29s)
        Expected false to be truthy.
        /home/runner/work/tapioca/tapioca/spec/spec_with_project.rb:98:in `assert_project_file_exist'
        /home/runner/work/tapioca/tapioca/spec/tapioca/cli/gem_spec.rb:[79](https://github.com/Shopify/tapioca/actions/runs/8388995127/job/22974281768?pr=1835#step:6:80)2:in `block (3 levels) in <class:GemSpec>'

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thank you, this is an important omission to address. I have a few comments/questions before we can merge this, though.

lib/tapioca/dsl/compilers/protobuf.rb Outdated Show resolved Hide resolved
spec/tapioca/dsl/compilers/protobuf_spec.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/protobuf.rb Outdated Show resolved Hide resolved
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the changes. This looks great now! Thanks again for the contribution.

@paracycle
Copy link
Member

I don't know what's up with the tests, but I'll merge this since they don't seem to be related at all.

@paracycle paracycle merged commit 2fa2a17 into main Mar 22, 2024
17 of 33 checks passed
@paracycle paracycle deleted the probotuf-gen-has-methods branch March 22, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants