Passion/gem rename #1
Conversation
| adapter_class = if defined? ActiveRecord::ConnectionAdapters::MySQLJdbcConnection | ||
| ActiveRecord::ConnectionAdapters::MySQLJdbcConnection | ||
| # elsif defined? ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter | ||
| # ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter |
There was a problem hiding this comment.
I'm curious about why the above are commented out. Do you think they'll come back when all the appraisals are filled in?
There was a problem hiding this comment.
I'm curious too. I've gone ahead and created the ticket for adding in the appraisals work, so in that ticket this code can either be uncommented or removed.
| col = cols[name.to_s] | ||
| raise ArgumentError, "Cannot find column #{name}" unless col | ||
|
|
There was a problem hiding this comment.
It looks like this is a copy of the ActiveRecord method that just picks off enums? Does the rest of the method still match Rails? Could this be redone as a prepend module that handles enums and then calls super for any other column types?
There was a problem hiding this comment.
I was looking and I don't think rails ever had (or at least I could find no documentation around) a validates_columns method. This looks to be entirely isolated to this gem. I think we can do better here for sure, but I'd like to put some thought into it rather than blocking the initial release for backwards compatibility,
There was a problem hiding this comment.
Oh, weird! I thought that name seemed so like the rest of the Rails methods...but oddly, it wasn't one I'd heard of.
So it's non-standard... and buggy. I don't think we need to keep it since this is a new gem anyway. I do like keeping the valuable part--enum inclusion--with the bug fixed. How about a name like
validate_enum_inclusion column_name
? (Or possibly a list of columns...but I'm not sure we need that.) Later we can plumb it from HoboFields when I add in my passion project that calls validations. Some Rails examples of doing the validation longhand: https://stackoverflow.com/questions/8146965/how-do-i-specify-and-validate-an-enum-in-rails
There was a problem hiding this comment.
Yea that sounds like a great idea. It will require a usage change in some of our usages internally in our main application, but I think that's more a reason to start making those changes now, and put them in a 1.0.0 release of the new gem
| # Test various known types. | ||
| case col.type | ||
| when :enum | ||
| validates_inclusion_of name, :in => col.limit, :allow_nil => true |
There was a problem hiding this comment.
This looks like a bug that it passes allow_nil: true. Shouldn't it plumb the flag from col.null?
validates_inclusion_of name, :in => col.limit, :allow_nil => col.null
There was a problem hiding this comment.
It does look like a bug, but without a great way to run tests, I'd like to leave the code as is right now, and work to improve it once we've got unit tests up and running. I've created a ticket already to add such unit tests and modernize the project a bit.
There was a problem hiding this comment.
Ok, I'm fine with the fix coming in then.
|
|
||
| when :integer, :float | ||
| validates_numericality_of name, :allow_nil => true |
There was a problem hiding this comment.
Huh, these other allow_nil: true settings look wrong too. Weird.
There was a problem hiding this comment.
Agreed, are you comfortable leaving as is for now while we get this in use and improve when we have unit tests up and running?
There was a problem hiding this comment.
Sure. And per my later comment above, in the next pass, I think we should remove this buggy code that doesn't involve enums!
Add basic automation
add rails 4 compatibility
[0.1.0] - Unreleased
Changed
enum_column3toactiverecord-mysql-enum