Open voltechs opened 2 years ago
Hi @voltechs - thanks for getting in touch, and it's great to know you find Combustion so helpful!
Also: I'm very keen to make additional database support possible. I suspect the freeze
is there due to Rubocop, but disabling that rule sounds like a fair move to me. Did you want to try using a local copy of Combustion, remove that freeze
call on OPERATOR_PATTERNS
and then add Snowflake into the hash and see if that works? If so, you're welcome to submit a PR, or otherwise I can make the adjustment - I just want to be sure it does what you need before publishing a new release :)
Hi @pat, that would definitely be a step in the right direction. Of course, OPERATOR_PATTERNS
is still a const, and so adding to it would at the very least produce a warning and maybe not the best approach in the end. Perhaps a class variable that one could append to at some point in the initialization process?
class Combustion::Database::Reset
# ...
@@operator_patterns = {
Combustion::Databases::MySQL => [/mysql/],
Combustion::Databases::PostgreSQL => [/postgres/, /postgis/],
Combustion::Databases::SQLite => [/sqlite/],
Combustion::Databases::SQLServer => [/sqlserver/],
Combustion::Databases::Oracle => %w[ oci oracle ],
Combustion::Databases::Firebird => %w[ firebird ]
}
# ...
def operator_class(adapter)
klass = nil
@@operator_patterns do |operator, keys|
klass = operator if keys.any? { |key| adapter[key] }
end
return klass if klass
raise UnsupportedDatabase, "Unsupported database type: #{adapter}"
end
end
Also, off-topic: would it make sense to simplify the operator_class
method as:
def operator_class(adapter)
@@operator_patterns do |operator, keys|
return operator if keys.any? { |key| adapter[key] }
end
raise UnsupportedDatabase, "Unsupported database type: #{adapter}"
end
Especially after opening up the hash to modification, there's a chance of traversing the whole hash even after finding a match.
Given we can rely on ActiveSupport being present, perhaps cattr_reader
could be used instead?
cattr_reader :operator_patterns, default: {
Combustion::Databases::MySQL => [/mysql/],
Combustion::Databases::PostgreSQL => [/postgres/, /postgis/],
Combustion::Databases::SQLite => [/sqlite/],
Combustion::Databases::SQLServer => [/sqlserver/],
Combustion::Databases::Oracle => %w[ oci oracle ],
Combustion::Databases::Firebird => %w[ firebird ]
}
Your simplification of operator_class
looks good to me 👍🏻
Also, I'm wondering if this attribute, the operator_class
method, and the UnsupportedDatabase
definition should be shifted to Combustion::Database
, with the method becoming a class-level concern, and the error class being renamed to UnsupportedDatabaseError
? If you'd rather hold off on that for now, I can look at doing that refactoring myself - but more than happy for you to sort it out if you'd like! :)
Sounds good. I'll try to take a stab at creating a PR here when I've got a few spare cycles!
Hi @pat!
I'm such a huge fan of Combustion; I was working on a new project and was looking to leverage Combustion for it.
Sadly I may be at an impasse. I'm trying to use the Snowflake DB, through a few different gems and configs. All in vein though, it would seem, as Combustion seems to be pretty stubborn on what databases it supports.
That is to say, without a substantial amount of "hacking", combustion has it's list of supported databases pretty locked down, unavailable for modification or extension from end users/developers.
Specifically: https://github.com/pat/combustion/blob/8916f6bec7f1a3de6baa7a15670f3b171d2238ba/lib/combustion/database/reset.rb#L9-L16 and https://github.com/pat/combustion/blob/8916f6bec7f1a3de6baa7a15670f3b171d2238ba/lib/combustion/database/reset.rb#L52-L60
Any thoughts on making that a little less rigid and available for others to append/modify as they might see fit?
Thanks for the consideration in advance 🙂