Rails 7 support#33
Conversation
|
|
||
| module ActiveRecord | ||
| module ConnectionAdapters | ||
| class AbstractMysqlAdapter |
There was a problem hiding this comment.
I think this is reopening the original adapter, is that right? Is that because there are effectively no hooks we could use? Or would this potentially be a good time to move to a proper initializer that hooks the adapter at that point?
There was a problem hiding this comment.
I wonder if maybe the thing to do is keep the register_enum_type method here (there are other, similar methods in the adapter already) and move the call of it to a Railtie or something
There was a problem hiding this comment.
My inclination is to get this PR in and then try to mess with that separately?
| class << self | ||
| def register_enum_type(mapping) | ||
| mapping.register_type(%r(enum)i) do |sql_type| | ||
| Type::Enum.new(limit: sql_type.scan(/'(.*?)'/).flatten) |
There was a problem hiding this comment.
Do you understand this limit code? I know it's copy/pasted, but the limit is quite opaque to me.
There was a problem hiding this comment.
The third party tool does this: limit = sql_type.sub(/^enum\('(.+)'\)/i, '\1').split("','").map { |v| v.to_sym } which is actually different (our code ends up with strings, the other goes into symbols)
I think the sql_type comes in here as a string formatted like enum('low', 'normal', 'high') and this is pulling the enum values into a Ruby array. I'm not 100% sure why you need to do that, but I think it's what makes the ActiveRecord conversion work.
| priority: [:enum, limit: priorities]) do | ||
| sql_enum :status | ||
| sql_enum :priority, _prefix: false, _suffix: true | ||
| sql_enum :status # duplicate should be no-op |
There was a problem hiding this comment.
Is there a way to directly test that this is in fact a no-op? I'm worried about the possibility of someone re-declaring using different options, then not understanding why their declaration doesn't work.
In fact, should this raise?
There was a problem hiding this comment.
Open to suggestions, this was from @dpep's PR, so maybe he wants to opine...
4 participants
Creates a new version just to support Rails 7.
In Rails 7, the type maps are created as class methods, not instance methods, and for our purposes that was a problem because it meant (I think) that the maps were loaded before our monkey patch was added.
In this version (based on Invoca/activerecord-mysql-enum#31), we explicitly call the type map and register the enum type.
This PR incorporates a couple of minor code cleanups from previous PRs that I closed rather than add them to the older version. (#32 from @tjwallace and #16 from @dpep)