Use keyword arguments for performance #24519
Conversation
| result &&= options[:from] == changed_attributes[attr] if options.key?(:from) | ||
| end | ||
| result &&= to == __send__(attr) unless to == :none | ||
| result &&= from == changed_attributes[attr] unless from == :none |
There was a problem hiding this comment.
Technically :none could be a valid to/from value. We'd need a guaranteed unique sentinel value, like OMITTED = Object.new. That starts looking pretty bad though.
Our other choice is **options, but then we lose the if options short-circuit when none are provided.
There was a problem hiding this comment.
Hard to imagine the case where attribute value is a symbol. Definitely not the case of AR.
bogdan
force-pushed
the
diry-keyword-args
branch
2 times, most recently
from
8e9fa41
to
5e3c868
Compare
|
@jeremy used a more exotic symbol to describe the option omission to make impossible to randomly match to real value. |
| @@ -174,12 +174,10 @@ def changed_attributes | |||
| end | |||
|
|
|||
| # Handles <tt>*_changed?</tt> for +method_missing+. | |||
| def attribute_changed?(attr, options = nil) #:nodoc: | |||
| def attribute_changed?(attr, from: :__none__, to: :__none__) # :nodoc: | |||
There was a problem hiding this comment.
Can we use a poison object here instead so that all possible user provided values are valid?
There was a problem hiding this comment.
bogdan
force-pushed
the
diry-keyword-args
branch
from
5e3c868
to
c251fb1
Compare
| @@ -119,6 +119,9 @@ module Dirty | |||
| extend ActiveSupport::Concern | |||
| include ActiveModel::AttributeMethods | |||
|
|
|||
| OPTION_NOT_GIVEN = Object.new | |||
bogdan
force-pushed
the
diry-keyword-args
branch
2 times, most recently
from
fac9642
to
c202944
Compare
bogdan
force-pushed
the
diry-keyword-args
branch
from
c202944
to
afb1f32
Compare
|
The failures are valid. We'll still need the |
| @@ -189,7 +189,7 @@ def attribute_was(attr) # :nodoc: | |||
| end | |||
|
|
|||
| # Handles <tt>*_previously_changed?</tt> for +method_missing+. | |||
| def attribute_previously_changed?(attr, options = {}) #:nodoc: | |||
| def attribute_previously_changed?(attr) #:nodoc: | |||
There was a problem hiding this comment.
What's the story re. changing the method signature? Is it related to this PR or just seem superfluous? Is it used/overridden/expected elsewhere?
There was a problem hiding this comment.
@vipulnsward did it in his alternative PR for some reason and recommended it to be used in mine. Sorry, I didn't think it through myself. @vipulnsward can you explain it?
There was a problem hiding this comment.
@jeremy The options are not passed to, or used here.
There was a problem hiding this comment.
Looks like the arg was added for parity with the attribute_changed? method signature, despite being unused. It's only been present through 5.0.0 betas. Think we're good to drop it. If the method takes args later, they'll be keyword args anyway.
Fixed bug introduced in #24519. Makes build green again
Following up for #24511 (comment)
r? @jeremy