Merge pull request #59 from cheddar-me/use-system-basic-object

Add support for rails 7.2

.github/workflows/test.yml

@@ -14,21 +14,12 @@ jobs:
       fail-fast: false
       matrix:
         ruby: ["3.0", "3.1", "3.2", "3.3"]
-
-        gemfile: [ "rails_6_1", "rails_7_0"]
+        gemfile: [ "rails_6_1", "rails_7_0", "rails_7_2"]
+        exclude:
+          - ruby: "3.0"
+            gemfile: "rails_7_2"
         experimental: [false]
 
-        #include:
-        #  - ruby: '2.7'
-        #    gemfile: rails_head
-        #    experimental: true
-        #  - ruby: '3.0'
-        #    gemfile: rails_head
-        #    experimental: true
-        #  - ruby: '3.1'
-        #    gemfile: rails_head
-        #    experimental: true
-
     steps:
       - uses: actions/checkout@v3
 

Appraisals

@@ -6,6 +6,6 @@ appraise "rails-7-0" do
   gem "rails", "~> 7.0.0"
 end
 
-appraise "rails-7-1" do
-  gem "rails", "~> 7.1.0"
-end
+appraise "rails-7-2" do
+  gem "rails", "~> 7.2.0"
+end

CHANGELOG.md

@@ -3,6 +3,11 @@ All notable changes to this project will be documented in this file.
 
 This format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## 0.18.0
+### Added
+- Add support for rails 7.2, but leave out rails 7.1 support. This is because ActionView has a breaking bug in 7.1 that renders the template back as a string
+instead of an object, like we need for Pbbuilder https://github.com/rails/rails/pull/51023
+
 ## 0.17.0
 ### Changed
 - Instead of appending to repeated enum message, we're replacing it to avoid issues in case output will be rendered twice

Gemfile

@@ -7,5 +7,3 @@ gemspec
 
 gem "rake"
 gem "appraisal"
-
-gem "ruby-lsp"

README.md

@@ -1,22 +1,19 @@
 # Pbbuilder
-PBBuilder generates [Protobuf](https://developers.google.com/protocol-buffers) Messages with a simple DSL similar to [JBuilder](https://rubygems.org/gems/jbuilder) gem.
+PBBuilder generates [Protobuf](https://developers.google.com/protocol-buffers) Messages with a simple DSL similar to the [JBuilder](https://rubygems.org/gems/jbuilder) gem.
 
 
-At least Rails 6.1 is required.
-
-> [!WARNING]  
-> There currently is a regression in ActionView (the part of Rails which renders) that forces rendered objects into strings. This is only present
-> in Rails 7.1, and is fixed in Rails. However, a 7.1 gem containing the fix hasn't been released yet. For the moment you should refrain
-> from using pbbuilder and rails-twirp with Rails 7.1 and wait for the next version to be released.
+At least Rails 6.1 is required and rails 7.1 is currently not supported.
+There currently is a regression in ActionView (the part of Rails which renders) that forces rendered objects into strings, but for Pbbuilder we need the raw objects.
+This is only present in Rails 7.1, and a fix is released in Rails 7.2. https://github.com/rails/rails/pull/51023
 
 ## Compatibility with jBuilder
 We don't aim to have 100% compitability and coverage with jbuilder gem, but we closely follow jbuilder's API design to maintain familiarity.
 
 | | Jbuilder | Pbbuilder |
 |---|---|---|
-|  set! | ✅ | ✅ |
-|  cache! | ✅ | ✅ |
-|  cache_if! | ✅ | ✅ |
+| set! | ✅ | ✅ |
+| cache! | ✅ | ✅ |
+| cache_if! | ✅ | ✅ |
 | cache_root! | ✅|  |
 | fragment cache | ✅| ✅ |
 | extract! | ✅ | ✅ |
@@ -25,7 +22,7 @@ We don't aim to have 100% compitability and coverage with jbuilder gem, but we c
 | array! | ✅ |  |
 | .call | ✅ |  |
 
-Due to protobuf message implementation, there is absolutely no need to implement support for `deep_format_keys!`, `key_format!`, `key_format`, `deep_format_keys`, `ignore_nil!`, `ignore_nil!`, `nil`. So those would never be added.
+Due to the protobuf message implementation, there is absolutely no need to implement support for `deep_format_keys!`, `key_format!`, `key_format`, `deep_format_keys`, `ignore_nil!`, `ignore_nil!`, `nil`. So those would never be added.
 
 ## Usage
 The main difference is that it can use introspection to figure out what kind of protobuf message it needs to create.
@@ -61,7 +58,7 @@ pb.phone_number account.phone_number
 pb.tag account.tag
 ```
 
-could be rewritten to a shorter version with a use of `extract!`.
+can be rewritten to a shorter version with the use of `extract!`.
 ```
 pb.extract! account, :id, :phone_number, :tag
 ```
@@ -80,7 +77,7 @@ Using partial while passing a variable to it
 pb.account partial: "account", account: @account
 ```
 
-Here is way to use partials with collection while passing a variable to it
+Here is a way to use partials with a collection while passing a variable to it
 
 ```
 pb.accounts @accounts, partial: "account", as: account
@@ -96,7 +93,7 @@ pb.friends partial: "racers/racer", as: :racer, collection: @racers
 pb.friends "racers/racer", as: :racer, collection: @racers
 ```
 
-And there are other ways, that don't use Collection Renderer (not very effective probably)
+And there are other ways, that don't use CollectionRenderer
 ```ruby
 pb.partial! @racer, racer: Racer.new(123, "Chris Harris", friends)
 ```
@@ -105,7 +102,7 @@ pb.friends @friends, partial: "racers/racer", as: :racer
 ```
 
 ### Caching
-it uses Rails.cache and works like caching in HTML templates:
+It uses Rails.cache and works like caching in HTML templates:
 
 ```
 pb.cache! "cache-key", expires_in: 10.minutes do
@@ -121,7 +118,7 @@ pb.cache_if! !admin?, "cache-key", expires_in: 10.minutes do
 end
 ```
 
-Fragment caching currently works through ActionView::CollectionRenderer and can be used only with the following syntax:
+Fragment caching currently works through ActionView::CollectionRenderer and can only be used with the following syntax:
 
 ```ruby
 pb.friends partial: "racers/racer", as: :racer, collection: @racers, cached: true
@@ -151,10 +148,14 @@ $ gem install pbbuilder
 
 When debugging, make sure to prepend `::Kernel` to any calls such as `puts` as otherwise the code will think you're trying to add another attribute into protobuf object.
 
-In case, you're looking to use breakpoints for debugging purposes - it's better to use `pry`. Just make sure to [change pbbuilder superclass from `ProxyObject/BasicObject` to `Object`](lib/pbbuilder/pbbuilder.rb).
+In case you're looking to use breakpoints (for debugging purposes via `binding.pry` for instance), let Pbbuilder inherit from `Object` instead of `BasicObject`]
+Seen in:
+[Pbbuilder](lib/pbbuilder/pbbuilder.rb)
+[Errors](lib/pbbuilder/errors.rb)
 
 ## Testing
-Running `bundle exec appraisal rake test` locally will run entire testsuit with all version of rails. To run tests only for certain rails version do the following `bundle exec appraisal rails-7-0 rake test`
+Running `bundle exec appraisal rake test` locally will run the entire testsuit with all versions of rails.
+To run tests only for a certain rails version do the following `bundle exec appraisal rails-7-0 rake test`
 
 To run only one tests from file - use `m` utility. Like this:
 `bundle exec appraisal rails-7-0 m test/pbbuilder_template_test.rb:182`

Rakefile

@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require "bundler/setup"
 require "bundler/gem_tasks"
 require "rake/testtask"
 
@@ -11,10 +10,28 @@ if !ENV["APPRAISAL_INITIALIZED"] && !ENV["CI"]
 else
   Rake::TestTask.new(:test) do |t|
     t.libs << "test"
-    t.pattern = "test/**/*_test.rb"
-    t.verbose = false
-    t.warning = false
+    t.libs << "lib"
+
+    # Running specific tests with line numbers, like with rails test, is not supported by default in rake.
+    # By setting the TESTOPS env var we can however specify the name of a single test with underscores instead of spaces.
+    # So run your single test by calling for ex:
+    #
+    # rake test /Users/sebastian/projects/cheddar/rails-twirp/test/ping_controller_test.rb "uncaught errors should bubble up to the test"
+
+    file_name = ARGV[1]
+    test_name = ARGV[2]&.tr(" ", "_")
+
+    ENV["TESTOPTS"] = "--verbose"
+
+    t.test_files = if file_name
+      if test_name
+        ENV["TESTOPTS"] += " --name=test_#{test_name}"
+      end
+      [file_name]
+    else
+      FileList["test/**/*_test.rb"]
+    end
   end
 
   task default: :test
-end
+end

gemfiles/rails_6_1.gemfile

@@ -4,7 +4,6 @@ source "https://rubygems.org"
 
 gem "rake"
 gem "appraisal"
-gem "ruby-lsp"
 gem "rails", "~> 6.1.0"
 
 gemspec path: "../"

gemfiles/rails_7_0.gemfile

@@ -4,7 +4,6 @@ source "https://rubygems.org"
 
 gem "rake"
 gem "appraisal"
-gem "ruby-lsp"
 gem "rails", "~> 7.0.0"
 
 gemspec path: "../"

gemfiles/rails_7_1.gemfile → gemfiles/rails_7_2.gemfile

@@ -4,7 +4,6 @@ source "https://rubygems.org"
 
 gem "rake"
 gem "appraisal"
-gem "ruby-lsp"
-gem "rails", "~> 7.1.0"
+gem "rails", "~> 7.2.0"
 
 gemspec path: "../"

lib/pbbuilder.rb

@@ -1,12 +1,10 @@
 # frozen_string_literal: true
 
-require "pbbuilder/pbbuilder"
-require 'pbbuilder/errors'
+require "pbbuilder/errors"
 require "pbbuilder/protobuf_extension"
 require "pbbuilder/railtie" if defined?(Rails)
 
-
-# Pbbuilder makes it easy to create a protobuf message using the builder pattern
+# Pbbuilder makes it easy to create a protobuf message using the builder pattern.
 # It is heavily inspired by jbuilder
 #
 # Given this example message definition:
@@ -15,8 +13,10 @@
 #   repeated Person friends = 2;
 # }
 #
-# You could use Pbbuilder as follows:
+# You can use Pbbuilder as follows:
+#
 # person = RPC::Person.new
+#
 # Pbbuilder.new(person) do |pb|
 #   pb.name "Hello"
 #   pb.friends [1, 2, 3] do |number|
@@ -29,7 +29,7 @@
 # It basically works exactly like jbuilder. The main difference is that it can use introspection to figure out what kind
 # of protobuf message it needs to create.
 
-class Pbbuilder
+class Pbbuilder < BasicObject
   def initialize(message)
     @message = message
 
@@ -48,11 +48,15 @@ def respond_to_missing?(field)
     !!_descriptor_for_field(field)
   end
 
+  # When calling for ex: response.drivers, where response is a Google::Protobuf object, 'drivers' is not a method on that. These
+  # methods (or messages in our case) get added here. This is of course based on what kind of message it is. Singular, an array
+  # (repeated) etc. with their arguments.
   def set!(field, *args, &block)
     name = field.to_s
     descriptor = _descriptor_for_field(name)
-    ::Kernel.raise ::ArgumentError, "Unknown field #{name}" if descriptor.nil?
+    ::Kernel.raise ::ArgumentError, "Unknown field: #{name}" if descriptor.nil?
 
+    # An block is used to pass on it's children
     if ::Kernel.block_given?
       ::Kernel.raise ::ArgumentError, "can't pass block to non-message field" unless descriptor.type == :message
 
@@ -66,9 +70,11 @@ def set!(field, *args, &block)
         # example syntax that should end up here:
         #   pb.field { pb.name "hello" }
         ::Kernel.raise ::ArgumentError, "wrong number of arguments (expected 0)" unless args.empty?
+
         message = (@message[name] ||= _new_message_from_descriptor(descriptor))
         _scope(message, &block)
       end
+    # No block given, but with 1 argument
     elsif args.length == 1
       arg = args.first
       if descriptor.label == :repeated
@@ -106,7 +112,7 @@ def set!(field, *args, &block)
       else
         # example syntax that should end up here:
         #   pb.field "value"
-        
+
         @message[name] = arg
       end
     else
@@ -128,6 +134,8 @@ def set!(field, *args, &block)
     end
   end
 
+  # Shorthand command for getting a few attributes from an object.
+  #     pb.extract! racer, :name, :id, :age
   def extract!(element, *args)
     args.each { |arg| @message[arg.to_s] = element.send(arg) }
   end
@@ -181,6 +189,7 @@ def target!
     @message
   end
 
+  # @param field string
   def new_message_for(field)
     descriptor = _descriptor_for_field(field)
     ::Kernel.raise ::ArgumentError, "Unknown field #{field}" if descriptor.nil?
@@ -190,11 +199,14 @@ def new_message_for(field)
 
   private
 
+  # Lookup the field name (or 'attribute' name, for ex "best_friend") on the Google descriptor (Google::Protobuf::Descriptor) of our
+  # message object.
+  # @param field string
   def _descriptor_for_field(field)
     @message.class.descriptor.lookup(field.to_s)
   end
 
-  # Appends protobuf message with existing @message object
+  # Appends protobuf objects to our 'repeated' attribute. This can create a list of items to a repeated field.
   #
   # @param name string
   # @param descriptor Google::Protobuf::FieldDescriptor
@@ -210,7 +222,8 @@ def _append_repeated(name, descriptor, collection, &block)
     @message[name].push(*elements)
   end
 
-  # Yields an Protobuf object in a scope of message and provided values.
+  # Yields a Protobuf object in the scope of message and provided values.
+  # This will 'assign' the field values as it were to the message attributes.
   #
   # @param message Google::Protobuf::(field_type)
   def _scope(message)
@@ -222,7 +235,7 @@ def _scope(message)
     @message = old_message
   end
 
-  # Build up empty protobuf message based on descriptor
+  # Builds up an empty protobuf message based on the given descriptor.
   #
   # @param descriptor Google::Protobuf::FieldDescriptor
   def _new_message_from_descriptor(descriptor)

lib/pbbuilder/errors.rb

@@ -1,12 +1,10 @@
 # frozen_string_literal: true
 
-require 'pbbuilder'
-
-class Pbbuilder
+class Pbbuilder < BasicObject
   class MergeError < ::StandardError
     def self.build(current_value, updates)
       message = "Can't merge #{updates.inspect} into #{current_value.inspect}"
       self.new(message)
     end
   end
-end
+end