-
Notifications
You must be signed in to change notification settings - Fork 113
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
Check for constant definition before invoking #1928
Conversation
@@ -102,7 +102,7 @@ def type_for_activerecord_value(column_type) | |||
"::Money" | |||
when ActiveRecord::Type::Integer | |||
"::Integer" | |||
when ActiveRecord::Encryption::EncryptedAttributeType | |||
when defined?(ActiveRecord::Encryption) && ActiveRecord::Encryption::EncryptedAttributeType |
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.
Would it be better to check for the exact constant that we reference?
when defined?(ActiveRecord::Encryption) && ActiveRecord::Encryption::EncryptedAttributeType | |
when defined?(ActiveRecord::Encryption::EncryptedAttributeType) && ActiveRecord::Encryption::EncryptedAttributeType |
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 think it is dangerous to use when
like this:
irb> foo = nil
#=> nil
irb> case foo
irb> when defined?(Foo) && 42
irb> puts "Oh no"
irb> when 42
irb> puts "Here"
irb> end
Oh no
The current way only works because defined?(Foo) && Foo
evaluates to Foo
if it is defined, but it evaluates to nil
if it is not.
I think what we want is:
when defined?(ActiveRecord::Encryption) && ActiveRecord::Encryption::EncryptedAttributeType | |
when ->(type) { defined?(ActiveRecord::Encryption) && type == ActiveRecord::Encryption::EncryptedAttributeType } |
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 change should be applied to all the other cases that use this pattern as well.
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.
Had to slightly change it to use ===
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.
Yes, I got confused with type
and mistook it for the class itself, but it's actually an instance of the class we are checking against. Sorry for the misdirection.
How come this wasn't failing the CI on Rails 6.1? Are we missing a test for this? |
Ah, because we don't officially support Rails 6.1, so we don't test against it, that makes sense. |
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.
Thanks!
@@ -98,11 +98,13 @@ def column_type_for(column_name) | |||
sig { params(column_type: T.untyped).returns(String) } | |||
def type_for_activerecord_value(column_type) | |||
case column_type | |||
when defined?(MoneyColumn) && MoneyColumn::ActiveRecordType | |||
when ->(type) { defined?(MoneyColumn) && MoneyColumn::ActiveRecordType === type } |
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.
Interesting pattern, which answers the question:
Why does
Proc
respond to#===
?! When would you ever callsome_proc === some_arg
instead of justsome_proc.call(some_arg)
?!
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.
Yes, a lot of the #===
implementations are so that you can use them in when
clauses.
Motivation
Closes #1913
Implementation
Tests