adapters as objects (#961) #1010
Conversation
beckjake
force-pushed
the
feature/adapters-as-objects
branch
from
1a5bac5
to
63793b7
Compare
|
can you elaborate on:
|
| if quote_policy is None: | ||
| quote_policy = {} | ||
|
|
||
| quote_policy = dbt.utils.merge(config.quoting, quote_policy) |
|
|
||
| adapter = adapter_type(config) | ||
| _ADAPTERS[adapter_name] = adapter | ||
| return adapter |
There was a problem hiding this comment.
very into this. since we are getting more into the habit of using static classes as namespaces, should this be AdapterFactory.create() rather than dbt.adapters.factory.get_adapter()?
this PR is big enough, I don't want you to add it to this. but worth considering for the future.
There was a problem hiding this comment.
I'm not sure. I think this is pretty reasonable to keep as a module-level entity, I don't think making the factory an object would gain much. The interface is pretty simple right now and I think that's very valuable. This kind of model in Python is pretty common (logging.getLogger() is the canonical example, I suppose).
| self.quoting_config, | ||
| kwargs.pop('quote_policy', {}) | ||
| ) | ||
| return self.relation_type.create(*args, **kwargs) |
There was a problem hiding this comment.
i think i understand now -- if the primary API for creating relations is on the adapter instead of the relation, then we can seamlessly pass in the quoting config.
i need to think a little bit more about that.
There was a problem hiding this comment.
Yup, exactly! adapter.create_relation(...) or something.
Create adapters as singleton objects instead of classes, resolves #961.
Adapters now store the config on themselves when they're generated, and the factory returns singleton instances rather than types.
It does seem that the whole relation creation API could be changed to be a bit more streamlined - we could have relation creates go through the adapter directly and then we wouldn't need a proxy, but that would break a lot of existing macros. I don't think it's worth the effort if this proxy is good enough.
Most of this PR is converting
method(cls, config, ...)tomethod(self, ...). The most substantial aspects are the changes to the factory code and the wrapper/relations quoting logic - I spent a lot of time getting quoting-related tests to pass.