-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Rename AS::Callback#merge #18747
Rename AS::Callback#merge #18747
Conversation
@@ -373,7 +373,7 @@ def initialize(name, filter, kind, options, chain_config) | |||
def filter; @key; end | |||
def raw_filter; @filter; end | |||
|
|||
def merge(chain, new_options) | |||
def merge_skip_callback(chain, new_options) |
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.
Maybe merge_conditional_options
?
…ck. Renamed it to indicate what it actually does.
148e716
to
0928c3f
Compare
Sounds even better. Updated. |
@@ -373,7 +373,7 @@ def initialize(name, filter, kind, options, chain_config) | |||
def filter; @key; end | |||
def raw_filter; @filter; end | |||
|
|||
def merge(chain, new_options) | |||
def merge_conditional_options(chain, new_options) |
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.
As we are here, and merge_conditional_options
new_options
parameter can only have a if
or unless
option , I wonder if we should change this to keyargs: something like
def merge_conditional_options(chain, if: [], unless: [])
Not strong about tho, just a thought... @rafaelfranca WDYT?
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 see no reason not to do so.
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.
Is not if
a reserved word?
Callback#merge merges options for a callback skip instead of a callback. Renamed it to indicate what it actually does.