-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Use the generic type map object for mysql field lookups #15200
Conversation
/cc @senny |
module Type | ||
class HashLookupTypeMap < TypeMap # :nodoc: | ||
def lookup(type) | ||
@mapping[type].try(:call, type) || default_value |
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.
Could we not use try
? 🙏
@rafaelfranca Updated. |
Waiting the build run. |
@@ -317,23 +317,17 @@ def cast_value(value) | |||
end | |||
end | |||
|
|||
TYPES = {} | |||
class << self | |||
TYPES = ConnectionAdapters::Type::HashLookupTypeMap.new |
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 there a difference between putting a constant inside or outside the class << self
block?
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 that I know.
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.
delegate :register_type, to: :TYPES
will fail if it's outside of the class << self
block.
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 TYPES part of public api? if not, perhaps this should be class instance var instead (or cattr_reader)?
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 could not consider it part of the public API, but I'm also not opposed to that change.
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.
It is not. I'll make it with nodoc
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.
Oops, I thought I already merged this. @sgrif could you add nodoc comment?
@rafaelfranca Nodoc added. |
Use the generic type map object for mysql field lookups
No description provided.