Implement Enum Column checker #192
Conversation
There was a problem hiding this comment.
Should it be in enum_checkers folder or column_checkers? What's the difference?
Currently there are column_checkers/enum_value_checker.rb and enum_checkers/enum_type_checker.rb.
There was a problem hiding this comment.
Should it be in enum_checkers folder or column_checkers? What's the difference?
It depends on what you want to iterate in the "processor". One is iterating through columns, another through defined_enums.
In enum_value_checker, we check that column with the enum type has corresponding Ruby values (through inclusion validator or enum). So the subject is a column.
enum_type_checker.rb focuses on enum with the corresponding column type.
Therefore, I think it should be EnumChecker as you want to ensure that every enum is covered by the enum column.
|
|
||
| # ActiveRecord supports native enum type since version 7 and only for PostgreSQL | ||
| def preconditions | ||
| Helper.postgresql? && ActiveRecord::VERSION::MAJOR >= 7 && column.present? |
There was a problem hiding this comment.
I remember we discussed enabling this checker only for PostgreSQL 15+, but let me do some benchmarks for earlier versions as well. I think we should stick to "postgres and AR >=7" condition here.
There was a problem hiding this comment.
I remember we discussed enabling this checker only for PostgreSQL 15+, but let me do some benchmarks for earlier versions as well.
This would be awesome!
There was a problem hiding this comment.
I think we should stick to "postgres and AR >=7" condition here.
Yes, we can do that 👍
| module DatabaseConsistency | ||
| module Checkers | ||
| # This class checks that ActiveRecord enum is backed by enum column | ||
| class EnumColumnChecker < EnumChecker |
There was a problem hiding this comment.
Now, this checker intersects with EnumTypeChecker with a shorter list of "allowed" types. Before, we were okay with many types, but now we want to promote enum types.
I don't think we should have a single checker as this could make code harder, but (I believe) you would agree that having both checkers enabled simultaneously doesn't make much sense.
From one side, this looks more like a mode (strict/open). However, the gem doesn't support modes per checkers, and I'm wondering if we should have such (similar to what Rubocop has, for example).
If we don't go in that direction, maybe we should have an internal toggle system that turns off/on some checkers based on intersections. For example, if this checker is on, then EnumTypeChecker is off. However, if we go that way, I'm afraid we may end up with something too complex and unobvious.
With that said, I tend to have modes but I would like to hear your opinion on this matter.
|
Hi @toydestroyer, How are you doing? Do you need any help on this one? |
Closes #169
Details TBD