potiuk commented on a change in pull request #17423:
URL: https://github.com/apache/airflow/pull/17423#discussion_r682966113
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -70,6 +70,7 @@ def __init__(self, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.connection: Optional[Connection] = kwargs.pop("connection", None)
self.conn: connection = None
+ self.schema: Optional[str] = kwargs.pop("schema", None)
Review comment:
The problem is that having `schema` as attribute in DBApiHook will
encourage people to use it in their other operators deriving from DBApi. It
will be rather difficult to make sure that everyone is using it in "hasattr"
way. Even if we two remember it now, we will forget about it and other
committers who are not involved will not even know about this. We could
potentially automate a pre-commit check if the "DBApiHook" is accessed with
hasattr, but I think we will never be able to do it with 100% accuracy and I
think it's simply not worth it for that single field, that can be simply
replaced with single line for each operator that wants to use it (in __init__):
```
self.schema: Optional[str] = kwargs.pop("schema", None)
```
I think we should think about DBApiHook as "Public" API of Airflow and any
change to it should be very, very carefully considered.
Another option would be to add `>=Airflow 2.2` limitation to Postgres
operator (and any other operator that uses it), but again I think sacrificing
backwards compatibility in this case is simply not worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
