Barn 77 allow context overrides in helper methods #70

cgaroutte · 2025-11-15T00:22:17Z

In a previous PR, I had added the ability to plumb the parameter_start and parameter_end arguments through the find_tokens, find_formatters, and find_warnings methodsin order to override parameter on the underlyingContextobject used by these methods. In this current PR, I realized I also needed to override anotherContextparameter:allow_unmatched_delimiter_end. Instead of adding yet another set of parameters to these methods, I created a generalized hash parameter that can be used to selectively override Context` parameters.

Note

Adds context_overrides to find_tokens, find_formatters, and find_warnings (removing parameter_start/parameter_end), updates specs/docs, and bumps to a 3.0.0 prerelease.

API:
- Add context_overrides to ParameterSubstitution.find_tokens, find_formatters, find_warnings for overriding Context attributes.
- Remove parameter_start and parameter_end args from these methods.
- Introduce private build_context to construct Context with overrides.
Tests:
- Update specs to pass context_overrides for non-default delimiters; fix a typo in variable name.
Docs/Version:
- Update CHANGELOG.md with 3.0.0 (unreleased) notes reflecting the API changes.
- Bump version to 3.0.0-0 and update Gemfile.lock to 3.0.0.pre.0.

^{Written by Cursor Bugbot for commit c271a17. This will update automatically on new commits. Configure here.}

…elper methods

Copilot

Pull Request Overview

This PR refactors the parameter override mechanism in helper methods by replacing separate parameter_start and parameter_end arguments with a unified context_overrides hash parameter. This change improves the API design by consolidating context configuration into a single parameter, making it easier to extend with additional overrides in the future. This is a breaking change warranting the major version bump from 2.1.0 to 3.0.0.

Key Changes:

Replaced parameter_start and parameter_end parameters with context_overrides hash in find_tokens, find_formatters, and find_warnings methods
Added new private build_context helper method to construct context objects with optional overrides
Fixed typo in test variable name (expression_with_bad_paramss → expression_with_bad_params)

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

File	Description
lib/parameter_substitution.rb	Refactored helper methods to accept `context_overrides` hash instead of individual parameters; added `build_context` helper method
lib/parameter_substitution/version.rb	Bumped version to 3.0.0-0 for breaking API change
spec/lib/parameter_substitution_spec.rb	Updated all test cases to use new `context_overrides` parameter; fixed typo in variable name
CHANGELOG.md	Documented breaking changes for version 3.0.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cursor

This PR is being reviewed by Cursor Bugbot

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

cursor · 2025-11-17T17:33:00Z

lib/parameter_substitution.rb

+        mapping: mapping
+      }
+
+      ParameterSubstitution::Context.new(**base_options.merge(context_overrides || {}))


Bug: Explicit Parameters Silently Overridden

The context_overrides hash can silently override the explicit string_with_tokens and mapping parameters due to incorrect merge order. If a caller passes context_overrides: {input: 'x'} or context_overrides: {mapping: {...}}, these will override the method's positional and keyword arguments, causing the explicit parameters to be ignored without any error or warning.

Ty, robot. Fixed.

gmcgilvray · 2025-11-17T19:01:48Z

lib/parameter_substitution/version.rb


 class ParameterSubstitution
-  VERSION = "2.1.0"
+  VERSION = "3.0.0-1"


Is this a prerelease shorthand?

Yeah, when you bundle install it appears in the lockfile as 3.0.0.pre.1

ebertz

looks like a reasonable change. Might make sense to add add tests for overriding allow_unmatched_delimiter_end since that is one of the intended use cases, and maybe a test for the behavior when other unsupported options are passed in the overrides hash?

…examples

Copilot

Pull Request Overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-18T22:00:18Z

lib/parameter_substitution/version.rb


 class ParameterSubstitution
-  VERSION = "2.1.0"
+  VERSION = "3.0.0-2"


Version string 3.0.0-2 does not follow semantic versioning prerelease format. Should be 3.0.0.pre.2 or 3.0.0-pre.2 to match the pattern mentioned in PR description (3.0.0.pre.0).

Suggested change

VERSION = "3.0.0-2"

VERSION = "3.0.0.pre.2"

Copilot · 2025-11-18T22:00:19Z

lib/parameter_substitution.rb

+      end
    end

+


Extra blank line should be removed to maintain consistent spacing in the file.

Suggested change

Copilot

Pull Request Overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-18T22:08:22Z

lib/parameter_substitution.rb

+      valid_keys = %i[
+        required_parameters
+        parameter_start
+        parameter_end
+        destination_encoding
+        allow_unknown_replacement_parameters
+        allow_nil
+        allow_unmatched_parameter_end
+      ].to_set


The list of valid context override keys is hardcoded here, creating a maintenance burden. If new attributes are added to the Context class, this list must be manually updated. Consider deriving valid keys from the Context class itself (e.g., using Context's initializer parameters) to ensure consistency.

Suggested change

valid_keys = %i[

required_parameters

parameter_start

parameter_end

destination_encoding

allow_unknown_replacement_parameters

allow_nil

allow_unmatched_parameter_end

].to_set

valid_keys = ParameterSubstitution::Context

.instance_method(:initialize)

.parameters

.select { |type, _| type == :key || type == :keyreq }

.map { |_, name| name }

.to_set

This almost sounds like a cool idea, except for that fact that the list of parameters for initialize is not the same as the list of keys being validated against here....

cgaroutte · 2025-11-18T22:15:48Z

@ebertz I added an explicit validation for accepted override keys. I also consolidated the override tests into some shared examples for each method affected.

earl-at-invoca · 2025-11-25T00:59:41Z

lib/parameter_substitution.rb

+      valid_keys = %i[
+        required_parameters
+        parameter_start
+        parameter_end
+        destination_encoding
+        allow_unknown_replacement_parameters
+        allow_nil
+        allow_unmatched_parameter_end
+      ].to_set


@cgaroutte It would be better if this was defined as a class constant, so that it only needs to be processed once (array of symbols is instantiated, and then converted to a Set), instead of every time this method is called.

👍 Good point.

earl-at-invoca · 2025-11-25T01:16:49Z

spec/lib/parameter_substitution_spec.rb

+          end
+        end
+      end
+


@cgaroutte Nice use of shared examples here!

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T21:11:15Z

lib/parameter_substitution.rb

+    def validate_context_overrides!(context_overrides)
+      return if context_overrides.empty?
+
+      invalid_keys = context_overrides.keys.reject { |key| VALID_CONTEXT_OVERRIDE_KEYS.include?(key.to_sym) }


The validation allows both string and symbol keys in context_overrides, but the merge operation at line 109 (**context_overrides.merge(base_options)) will create duplicate keys if string keys are used. For example, if context_overrides contains {'input' => 'value'}, this validation will pass (because line 117 converts it to :input), but line 109 will result in both :input and 'input' keys being passed to Context.new. Consider either converting all keys to symbols before validation and merge, or explicitly rejecting string versions of reserved keys.

Copilot · 2025-11-25T21:11:16Z

spec/lib/parameter_substitution_spec.rb

+        context 'when context_overrides attempts to override base options' do
+          context "when input is provided in context_overrides" do
+            let(:test_context_overrides) { { input: "<different>" } }
+            it "raises error" do
+              expect { subject }.to raise_error(ArgumentError, /Invalid context_overrides keys: input/)
+            end
+          end
+
+          context "when mapping is provided in context_overrides" do
+            let(:test_context_overrides) { { mapping: { 'different' => 'value' } } }
+            it "raises error" do


The test expects input to be rejected as an invalid key, but input is not listed in VALID_CONTEXT_OVERRIDE_KEYS (lines 84-92 in parameter_substitution.rb), so it would already be rejected by the general validation logic. The current implementation doesn't specifically check for 'base options' (input and mapping); it only validates against the allowed keys list. This test will pass, but for the wrong reason. Consider either adding explicit validation for base options that provides a clearer error message, or updating the test description to reflect that these are simply invalid keys rather than special 'base options'.

Suggested change

context 'when context_overrides attempts to override base options' do

context "when input is provided in context_overrides" do

let(:test_context_overrides) { { input: "<different>" } }

it "raises error" do

expect { subject }.to raise_error(ArgumentError, /Invalid context_overrides keys: input/)

end

end

context "when mapping is provided in context_overrides" do

let(:test_context_overrides) { { mapping: { 'different' => 'value' } } }

it "raises error" do

context 'when context_overrides contains disallowed keys' do

context "when input is provided in context_overrides" do

let(:test_context_overrides) { { input: "<different>" } }

it "raises error for invalid key" do

expect { subject }.to raise_error(ArgumentError, /Invalid context_overrides keys: input/)

end

end

context "when mapping is provided in context_overrides" do

let(:test_context_overrides) { { mapping: { 'different' => 'value' } } }

it "raises error for invalid key" do

…ectations

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T21:42:38Z

lib/parameter_substitution.rb

+    # @raise [ArgumentError] if context_overrides contains invalid keys
+    def validate_context_overrides!(context_overrides)
+      return if context_overrides.empty?
+


The validation allows 'input' and 'mapping' keys to pass through if they're present in context_overrides, but these should be rejected since they're provided as base_options. Add explicit validation to reject these reserved keys before checking against VALID_CONTEXT_OVERRIDE_KEYS.

Suggested change

reserved_keys = %i[input mapping]

present_reserved_keys = context_overrides.keys.map(&:to_sym) & reserved_keys

if present_reserved_keys.any?

raise ArgumentError, "context_overrides contains reserved keys: #{present_reserved_keys.join(', ')}. These keys are not allowed."

end

🤔 They aren't included in VALID_CONTEXT_OVERRIDE_KEYS though so they're going to be rejected regardless...I don't think the extra complexity is warranted here for a slightly more specific error message.

cgaroutte added 2 commits November 14, 2025 15:39

BARN-77: allow context parameter overrides in ParameterSubstitution h…

278c40b

…elper methods

BARN-77: update README

c271a17

Copilot AI review requested due to automatic review settings November 15, 2025 00:22

Copilot AI reviewed Nov 15, 2025

View reviewed changes

cgaroutte marked this pull request as ready for review November 17, 2025 17:29

cursor bot reviewed Nov 17, 2025

View reviewed changes

BARN-77: don't override base context options

b560c84

cgaroutte requested review from a team, ebertz, gmcgilvray and oleksiy and removed request for a team and oleksiy November 17, 2025 18:09

gmcgilvray approved these changes Nov 17, 2025

View reviewed changes

ebertz reviewed Nov 18, 2025

View reviewed changes

cgaroutte added 2 commits November 18, 2025 13:59

BARN-77: validate context override keys; update specs and use shared_…

5b61af8

…examples

BARN-77: bump pre-release version

cd43b78

Copilot AI review requested due to automatic review settings November 18, 2025 21:59

Copilot AI reviewed Nov 18, 2025

View reviewed changes

cgaroutte added 2 commits November 18, 2025 14:01

BARN-77: remove extra blank line

1074915

BARN-77: remove unecessary extra shared_examples block

ddcc955

Copilot AI review requested due to automatic review settings November 18, 2025 22:07

Copilot AI reviewed Nov 18, 2025

View reviewed changes

cgaroutte requested a review from ebertz November 18, 2025 22:14

earl-at-invoca requested review from earl-at-invoca and removed request for ebertz November 25, 2025 00:33

earl-at-invoca reviewed Nov 25, 2025

View reviewed changes

spec/lib/parameter_substitution_spec.rb

end

end

end

Copy link

earl-at-invoca Nov 25, 2025

Choose a reason for hiding this comment

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

@cgaroutte Nice use of shared examples here!

cgaroutte reacted with rocket emoji

BARN-77: extract valid override keys into private class constant

8522639

cgaroutte requested a review from earl-at-invoca November 25, 2025 18:25

earl-at-invoca approved these changes Nov 25, 2025

View reviewed changes

BARN-77: bump to release version

4f58e13

Copilot AI review requested due to automatic review settings November 25, 2025 21:10

Copilot AI reviewed Nov 25, 2025

View reviewed changes

cgaroutte added 2 commits November 25, 2025 13:11

BARN-77: update CHANGELOG

4e522b8

BARN-77: add support for string override keys to match validation exp…

34f4af4

…ectations

Copilot AI review requested due to automatic review settings November 25, 2025 21:42

Copilot AI reviewed Nov 25, 2025

View reviewed changes

cgaroutte merged commit eae0148 into master Nov 25, 2025
39 of 40 checks passed

+      reserved_keys = %i[input mapping]
+      present_reserved_keys = context_overrides.keys.map(&:to_sym) & reserved_keys
+      if present_reserved_keys.any?
+        raise ArgumentError, "context_overrides contains reserved keys: #{present_reserved_keys.join(', ')}. These keys are not allowed."
+      end

Barn 77 allow context overrides in helper methods #70

Barn 77 allow context overrides in helper methods #70

Uh oh!

Conversation

cgaroutte commented Nov 15, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

This PR is being reviewed by Cursor Bugbot

Uh oh!

cursor bot Nov 17, 2025

Choose a reason for hiding this comment

Bug: Explicit Parameters Silently Overridden

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebertz left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgaroutte commented Nov 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

cgaroutte commented Nov 15, 2025 •

edited by cursor bot

Loading