From b70dbc760b694d6effacc074746ad37d067d90c8 Mon Sep 17 00:00:00 2001 From: Benjamin Hargett Date: Thu, 18 Dec 2025 09:43:36 -0500 Subject: [PATCH 1/4] feat!: change required to check key presence, add validates option --- .standard.yml | 2 +- lib/telephone/service.rb | 52 ++++++++++++++++++++++++++++++---------- spec/telephone_spec.rb | 40 ++++++++++++++++++++++++++----- 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/.standard.yml b/.standard.yml index a28f6c3..a7953f1 100644 --- a/.standard.yml +++ b/.standard.yml @@ -1,7 +1,7 @@ fix: true parallel: true format: progress -ruby_version: 2.6 +ruby_version: 3.4 ignore: - "bin/**/*" \ No newline at end of file diff --git a/lib/telephone/service.rb b/lib/telephone/service.rb index 295aeac..0402380 100644 --- a/lib/telephone/service.rb +++ b/lib/telephone/service.rb @@ -40,10 +40,20 @@ def initialize(attributes = {}) send("#{key}=", resolved) end + validate_required_arguments!(attributes) + super yield self if block_given? end + def validate_required_arguments!(attributes) + self.class.required_arguments.each do |arg| + unless attributes.key?(arg) + raise ArgumentError, "missing required argument: #{arg}" + end + end + end + ## # Determines whether or not the action of the service # object was successful. @@ -63,9 +73,12 @@ def call class << self ## - # Defines a getter/setter for a service object argument. This also allows you - # to pass in a default, or set the argument to "required" to add a validation - # that runs before executing the block. + # Defines a getter/setter for a service object argument. + # + # @param arg [Symbol] The name of the argument + # @param default [Object, Proc] Default value or callable that returns the default + # @param required [Boolean] If true, raises ArgumentError if argument is not provided + # @param validates [Hash] ActiveModel validation options to apply # # The default value can be a static value or any callable object (Proc, lambda, # method, or any object that responds to #call) that will be evaluated at @@ -75,24 +88,30 @@ class << self # so they can access other attributes. They are processed in definition order, # meaning a callable can depend on any argument defined before it. # + # The +required+ option checks if the argument key was provided, not if the + # value is present. Passing +nil+ explicitly satisfies the requirement. + # # To store a Proc as the actual value, wrap it in another lambda: # argument :my_proc, default: -> { -> { puts "hi" } } # - # @example + # @example Basic usage + # argument :name, default: "John" + # argument :name, required: true + # + # @example With validations + # argument :email, validates: { format: { with: /@/ } } + # argument :name, required: true, validates: { length: { minimum: 2 } } + # + # @example With callable defaults # class SomeService < Telephone::Service # argument :first_name, default: "John" # argument :last_name, default: "Doe" # argument :full_name, default: -> { "#{first_name} #{last_name}" } - # argument :timestamp, default: -> { DateTime.current } - # - # def call - # puts full_name - # puts timestamp - # end # end - def argument(arg, default: nil, required: false) - send(:attr_accessor, arg.to_sym) - send(:validates, arg.to_sym, presence: true) if required + def argument(arg, default: nil, required: false, validates: nil) + attr_accessor(arg.to_sym) + required_arguments << arg.to_sym if required + validates(arg.to_sym, validates) if validates defaults[arg.to_sym] = default end @@ -104,6 +123,13 @@ def defaults @defaults ||= {} end + ## + # List of arguments marked as required. Uses a nil check instead of + # presence validation to avoid triggering queries on ActiveRecord relations. + def required_arguments + @required_arguments ||= [] + end + ## # Executes the service object action directly from the class — similar to the # way a Proc can be executed with `Proc#call`. This allows us to add some common diff --git a/spec/telephone_spec.rb b/spec/telephone_spec.rb index 8c9e356..65c7e67 100644 --- a/spec/telephone_spec.rb +++ b/spec/telephone_spec.rb @@ -28,7 +28,7 @@ def call it "allows requiring an argument" do subject.public_send(:argument, :required_field, required: true) - expect(subject.call.success?).to be false + expect { subject.call }.to raise_error(ArgumentError) end context "with callable defaults" do @@ -168,11 +168,8 @@ def call context "if there is a required argument" do before { subject.public_send(:argument, :required_field, required: true) } - it "cannot call without the required argument" do - instance = subject.new - expect(instance.valid?).to be false - expect(instance.call).to be_a Telephone::Service - expect(instance.call.success?).to be false + it "raises ArgumentError when argument is not provided" do + expect { subject.new }.to raise_error(ArgumentError, "missing required argument: required_field") end it "works as expected with the required argument" do @@ -183,6 +180,37 @@ def call expect(instance.call.result).to be instance.foo expect(instance.call).to be_a Telephone::Service end + + it "allows nil as an explicit value" do + instance = subject.new(required_field: nil) + expect(instance.required_field).to be_nil + end + + it "allows false as an explicit value" do + instance = subject.new(required_field: false) + expect(instance.required_field).to be false + end + end + + context "with validates option" do + it "applies ActiveModel validations" do + service = Class.new(Telephone::Service) do + argument :email, validates: {format: {with: /@/}} + end + + expect(service.new(email: "invalid").valid?).to be false + expect(service.new(email: "valid@example.com").valid?).to be true + end + + it "can combine required with validates" do + service = Class.new(Telephone::Service) do + argument :name, required: true, validates: {length: {minimum: 2}} + end + + expect { service.new }.to raise_error(ArgumentError) + expect(service.new(name: "A").valid?).to be false + expect(service.new(name: "Al").valid?).to be true + end end end From 196e2e77ece9cfdca9dd3939ded1b2fd43d70947 Mon Sep 17 00:00:00 2001 From: Benjamin Hargett Date: Thu, 18 Dec 2025 09:57:01 -0500 Subject: [PATCH 2/4] Add rake task for migrating to v2.0 required behavior --- lib/telephone.rb | 1 + lib/telephone/railtie.rb | 9 +++ lib/telephone/tasks/migrate.rake | 44 ++++++++++++ spec/migrate_task_spec.rb | 119 +++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+) create mode 100644 lib/telephone/railtie.rb create mode 100644 lib/telephone/tasks/migrate.rake create mode 100644 spec/migrate_task_spec.rb diff --git a/lib/telephone.rb b/lib/telephone.rb index 296b05b..1aeef5c 100644 --- a/lib/telephone.rb +++ b/lib/telephone.rb @@ -1,3 +1,4 @@ # frozen_string_literal: true require "telephone/service" +require "telephone/railtie" if defined?(Rails::Railtie) diff --git a/lib/telephone/railtie.rb b/lib/telephone/railtie.rb new file mode 100644 index 0000000..56c5cb4 --- /dev/null +++ b/lib/telephone/railtie.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Telephone + class Railtie < Rails::Railtie + rake_tasks do + load "telephone/tasks/migrate.rake" + end + end +end diff --git a/lib/telephone/tasks/migrate.rake b/lib/telephone/tasks/migrate.rake new file mode 100644 index 0000000..f665b86 --- /dev/null +++ b/lib/telephone/tasks/migrate.rake @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +namespace :telephone do + desc "Migrate required: true to include validates: { presence: true } for v2.0 compatibility" + task :migrate, [:path] do |_t, args| + path = args[:path] || "app/services" + + unless Dir.exist?(path) + puts "Directory not found: #{path}" + puts "Usage: rake telephone:migrate[path/to/services]" + exit 1 + end + + files_updated = 0 + + Dir.glob("#{path}/**/*.rb").each do |file| + content = File.read(file) + original = content.dup + + # Match argument with required: true that doesn't already have validates: + content.gsub!(/^(\s*argument\s+:\w+.*)required:\s*true(.*)$/) do |line| + if line.include?("validates:") + line + else + indent = line[/^\s*/] + line.sub(/required:\s*true/, "required: true, validates: { presence: true }") + end + end + + if content != original + File.write(file, content) + puts "Updated: #{file}" + files_updated += 1 + end + end + + if files_updated == 0 + puts "No files needed updating." + else + puts "\nUpdated #{files_updated} file(s)." + puts "Review the changes and run your tests to verify." + end + end +end diff --git a/spec/migrate_task_spec.rb b/spec/migrate_task_spec.rb new file mode 100644 index 0000000..40c5f9a --- /dev/null +++ b/spec/migrate_task_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require "rake" +require "fileutils" +require "tmpdir" + +RSpec.describe "telephone:migrate rake task" do + let(:tmpdir) { Dir.mktmpdir } + + after do + FileUtils.rm_rf(tmpdir) + end + + def run_migrate(path = tmpdir) + load File.expand_path("../lib/telephone/tasks/migrate.rake", __dir__) + Rake::Task["telephone:migrate"].reenable + Rake::Task["telephone:migrate"].invoke(path) + end + + it "adds validates: { presence: true } to required: true arguments" do + File.write("#{tmpdir}/example_service.rb", <<~RUBY) + class ExampleService < Telephone::Service + argument :user, required: true + argument :name, default: "test" + + def call + user + end + end + RUBY + + run_migrate + + content = File.read("#{tmpdir}/example_service.rb") + expect(content).to include("argument :user, required: true, validates: { presence: true }") + expect(content).to include('argument :name, default: "test"') + end + + it "handles required: true with other options" do + File.write("#{tmpdir}/multi_option_service.rb", <<~RUBY) + class MultiOptionService < Telephone::Service + argument :email, required: true, default: nil + end + RUBY + + run_migrate + + content = File.read("#{tmpdir}/multi_option_service.rb") + expect(content).to include("required: true, validates: { presence: true }") + end + + it "does not modify arguments that already have validates:" do + original = <<~RUBY + class AlreadyMigratedService < Telephone::Service + argument :email, required: true, validates: { format: { with: /@/ } } + end + RUBY + + File.write("#{tmpdir}/already_migrated_service.rb", original) + + run_migrate + + content = File.read("#{tmpdir}/already_migrated_service.rb") + expect(content).to eq(original) + end + + it "does not modify arguments without required: true" do + original = <<~RUBY + class NoRequiredService < Telephone::Service + argument :name, default: "test" + argument :optional + end + RUBY + + File.write("#{tmpdir}/no_required_service.rb", original) + + run_migrate + + content = File.read("#{tmpdir}/no_required_service.rb") + expect(content).to eq(original) + end + + it "handles multiple required arguments in the same file" do + File.write("#{tmpdir}/multiple_required_service.rb", <<~RUBY) + class MultipleRequiredService < Telephone::Service + argument :user, required: true + argument :account, required: true + argument :optional_thing + end + RUBY + + run_migrate + + content = File.read("#{tmpdir}/multiple_required_service.rb") + expect(content).to include("argument :user, required: true, validates: { presence: true }") + expect(content).to include("argument :account, required: true, validates: { presence: true }") + expect(content).to include("argument :optional_thing") + expect(content).not_to include("optional_thing, required") + end + + it "preserves indentation" do + File.write("#{tmpdir}/indented_service.rb", <<~RUBY) + module Foo + class IndentedService < Telephone::Service + argument :user, required: true + + def call + user + end + end + end + RUBY + + run_migrate + + content = File.read("#{tmpdir}/indented_service.rb") + expect(content).to include(" argument :user, required: true, validates: { presence: true }") + end +end From d5c74d7f2fdaf9e497a72fdb77fca4cf1dba12da Mon Sep 17 00:00:00 2001 From: Benjamin Hargett Date: Thu, 18 Dec 2025 09:58:44 -0500 Subject: [PATCH 3/4] Update README for v2.0 changes --- README.md | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index cb97f8a..134ff1c 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ end SimpleExample.call(name: "Benjamin").result #=> "Hello, Benjamin." ``` -Arguments can also be required, which will prevent the service object from executing unless they are present. +Arguments can also be required, which will raise an `ArgumentError` if not provided: ```ruby class SimpleExample < ApplicationService @@ -82,10 +82,8 @@ class SimpleExample < ApplicationService end end -s = SimpleExample.call -s.success? #=> false -s.errors.full_messages #=> ["Name can't be blank"] -s.result #=> nil +SimpleExample.call #=> ArgumentError: missing required argument: name +SimpleExample.call(name: nil) #=> works - nil is a valid value ``` You can also give a default value for an argument. @@ -112,6 +110,13 @@ GreetingService.call.result #=> "Hello, John Doe!" GreetingService.call(first_name: "Jane").result #=> "Hello, Jane Doe!" ``` +Arguments can also have inline validations using the `validates:` option: + +```ruby +argument :email, validates: { format: { with: /@/ } } +argument :name, required: true, validates: { length: { minimum: 2 } } +``` + ### Validations Since `Telephone::Service` includes `ActiveModel::Model`, you can define validations in the same way you would for an ActiveRecord model. @@ -155,3 +160,16 @@ yard server --reload The `--reload`, or `-r`, flag tells the server to auto reload the documentation on each request. Once the server is running, the documentation will by available at http://localhost:8808 + +## Upgrading to 2.0 + +Version 2.0 changes the behavior of `required: true`. Previously it added a presence validation; now it raises `ArgumentError` if the argument key isn't provided. + +To migrate, run: + +```bash +rake telephone:migrate # defaults to app/services +rake telephone:migrate[path/to/services] # custom path +``` + +This adds `validates: { presence: true }` to preserve the old validation behavior. From 6dceef0eb5e14b56f5929fb0637b2c68f5e49385 Mon Sep 17 00:00:00 2001 From: Benjamin Hargett Date: Thu, 18 Dec 2025 10:01:14 -0500 Subject: [PATCH 4/4] Remove frozen_string_literal and dead code --- lib/telephone/railtie.rb | 2 -- lib/telephone/service.rb | 3 --- lib/telephone/tasks/migrate.rake | 4 ---- spec/migrate_task_spec.rb | 2 -- 4 files changed, 11 deletions(-) diff --git a/lib/telephone/railtie.rb b/lib/telephone/railtie.rb index 56c5cb4..17c55b0 100644 --- a/lib/telephone/railtie.rb +++ b/lib/telephone/railtie.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - module Telephone class Railtie < Rails::Railtie rake_tasks do diff --git a/lib/telephone/service.rb b/lib/telephone/service.rb index 0402380..22d740e 100644 --- a/lib/telephone/service.rb +++ b/lib/telephone/service.rb @@ -123,9 +123,6 @@ def defaults @defaults ||= {} end - ## - # List of arguments marked as required. Uses a nil check instead of - # presence validation to avoid triggering queries on ActiveRecord relations. def required_arguments @required_arguments ||= [] end diff --git a/lib/telephone/tasks/migrate.rake b/lib/telephone/tasks/migrate.rake index f665b86..89de3e7 100644 --- a/lib/telephone/tasks/migrate.rake +++ b/lib/telephone/tasks/migrate.rake @@ -1,5 +1,3 @@ -# frozen_string_literal: true - namespace :telephone do desc "Migrate required: true to include validates: { presence: true } for v2.0 compatibility" task :migrate, [:path] do |_t, args| @@ -17,12 +15,10 @@ namespace :telephone do content = File.read(file) original = content.dup - # Match argument with required: true that doesn't already have validates: content.gsub!(/^(\s*argument\s+:\w+.*)required:\s*true(.*)$/) do |line| if line.include?("validates:") line else - indent = line[/^\s*/] line.sub(/required:\s*true/, "required: true, validates: { presence: true }") end end diff --git a/spec/migrate_task_spec.rb b/spec/migrate_task_spec.rb index 40c5f9a..63eacf2 100644 --- a/spec/migrate_task_spec.rb +++ b/spec/migrate_task_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "rake" require "fileutils" require "tmpdir"