BARN-180: Add find_all_tokens method to parameter substitution to all…#73
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new find_all_tokens method to the ParameterSubstitution gem that recursively collects all parameter names including those nested inside method call arguments. This extends the functionality of the existing find_tokens method which only returns top-level parameter names.
Changes:
- Added
find_all_tokenspublic API method that accepts the same parameters asfind_tokens - Implemented
all_substitution_parameter_namesmethod in the Expression class that recursively traverses nested expressions - Version bumped from 3.1.0 to 3.2.0.pre.2
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/parameter_substitution.rb | Adds new public API method find_all_tokens following the same pattern as existing find_tokens method |
| lib/parameter_substitution/expression.rb | Implements recursive all_substitution_parameter_names method and helper methods to traverse nested method call arguments |
| lib/parameter_substitution/version.rb | Bumps version to 3.2.0.pre.2 |
| Gemfile.lock | Updates version reference to 3.2.0.pre.2 |
| spec/lib/parameter_substitution_spec.rb | Adds comprehensive tests for find_all_tokens including nested expressions, custom delimiters, and context_overrides validation |
| spec/lib/parameter_substitution/expression_spec.rb | Adds unit tests for all_substitution_parameter_names method covering various nesting scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class ParameterSubstitution | ||
| VERSION = "3.1.0" | ||
| VERSION = "3.2.0.pre.2" |
There was a problem hiding this comment.
The CHANGELOG.md file should be updated to document the new find_all_tokens method for version 3.2.0.pre.2. Following the pattern established in previous versions (e.g., 3.0.0), there should be a new section documenting this feature addition.
| VERSION = "3.2.0.pre.2" | |
| VERSION = "3.2.0.pre.1" |
There was a problem hiding this comment.
I agree that the changelog should be updated.
cgaroutte
left a comment
cgaroutte
left a comment
There was a problem hiding this comment.
Nice, I like the recursive approach here. I just had a few minor comments.
|
|
||
| it "returns nested parameter names from method call arguments" do | ||
| expression = parse_expression("<simple_text.if_nil(<another_simple_text>)>") | ||
| expect(expression.all_substitution_parameter_names).to eq(["simple_text", "another_simple_text"]) |
There was a problem hiding this comment.
Nit: would you mind adding a space between the expect statements and the test setup?
| parse_expression(context).substitution_parameter_names | ||
| end | ||
|
|
||
| def find_all_tokens(string_with_tokens, mapping: {}, context_overrides: {}) |
There was a problem hiding this comment.
Do you think we should get rid of the find_tokens method? I see it used in one other place in web other than the mapper (in TokenReplacement). It looks like it's extracting tokens from a webhook url template. It feels like a bug that we're only finding top-level tokens in that case as well.
There was a problem hiding this comment.
If that is a bug, we should hit the birds up to fix. I don't think we should concern ourselves with that behavior. Also, the url template might not support nested params (just a thought).
| parse_expression(context).substitution_parameter_names | ||
| end | ||
|
|
||
| def find_all_tokens(string_with_tokens, mapping: {}, context_overrides: {}) |
There was a problem hiding this comment.
A method doc might be nice here.
| end | ||
|
|
||
| def parameter_names_from_method_calls(method_calls) | ||
| return [] unless method_calls |
There was a problem hiding this comment.
I would prefer if this early return was an if/else.
There was a problem hiding this comment.
Yes, good find. I personally like these guard clauses for simple cases like this (strung together and multiple don't make as much sense to me). But I will change it!
|
|
||
| class ParameterSubstitution | ||
| VERSION = "3.1.0" | ||
| VERSION = "3.2.0.pre.2" |
There was a problem hiding this comment.
I agree that the changelog should be updated.
| def parameter_names_from_method_calls(method_calls) | ||
| return [] unless method_calls | ||
|
|
||
| method_calls.flat_map do |mc| |
There was a problem hiding this comment.
I am not a huge fan of shortened argument names like this because I find its trading legibility for brevity, but that is a personal preference so please ignore if you feel otherwise.
There was a problem hiding this comment.
Happy to change!
gmcgilvray
force-pushed
the
BARN-180-webhook-field-reference-mapper-does-not-account-for-nested-substitution-parameters
branch
2 times, most recently
from
a3cac31 to
85dbb1c
Compare
…ow grabbing nested expression params
gmcgilvray
force-pushed
the
BARN-180-webhook-field-reference-mapper-does-not-account-for-nested-substitution-parameters
branch
from
85dbb1c to
c1b829b
Compare
2 participants
…ow grabbing nested expression params