-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement nested with
support in parameter DSL
#2434
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
Conversation
def new_scope(args, _, &block) | ||
nested_scope = self.class.new | ||
nested_scope.new_group_scope(args, &block) | ||
nested_scope | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this, but looks legit for me.
Looks good. Document it in README please? Maybe add a test with 3 levels just to be sure ;) |
@@ -264,6 +264,7 @@ def new_scope(attrs, optional = false, &block) | |||
parent: self, | |||
optional: optional, | |||
type: type || Array, | |||
group: @group, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dblock I've added more specs and found that in case of mix of nested scopes and groups, with
doesn't work as expected (if there is anything expected). The reason is that when a new parameter with a block is defined , a new scope is created without any link to the current group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open an issue with a failed spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether it is an issue or an undocumented feature is always hard to tell in such mature projects :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow the rule of thumb of 1) is there a spec (undocumented feature), 2) is it in README (documented feature).
prev_group = @group | ||
@group = args.clone.first | ||
yield | ||
@group = prev_group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we do not really create scopes, this is my imitation of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, add to README?
b86686c
to
6bc75fc
Compare
Motivation
Previously, the
with
' method in the Parameter DSL did not support nested structures. This forced to redefine the entire attribute set for each nested scope, which was cumbersome and prone to errors, especially in complex parameter structures.Solution
This PR introduces enhanced functionality to the
with
method that allows nested scopes to inherit and extend the attributes defined in parent scopes. By merging attributes from the current group with the new attributes, it simplifies parameter declarations and ensures a clean, DRY approach to specifying shared attributes in nested parameter contexts.