xinbinhuang commented on a change in pull request #16521:
URL: https://github.com/apache/airflow/pull/16521#discussion_r655608121
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here is actually an instance attribute so it should
go inside the `__init__` call.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
I believe the schema here should be an instance attribute and it should
go inside the `__init__` call.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting
overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific
params? Also, all other superclass shouldn't worry about `self.schema =
<schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing right now because we're mixing class attribute
and instance attribute. What's the purpose of `schema` at the class level? When
we do `self.schema = None` in subclass, the class attribture `schema` will
simply get ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting
overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific
params? Also, all other superclass shouldn't worry about `self.schema =
<schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute.
What's the purpose of `schema` at the class level? When we do `self.schema =
None` in subclass, the class attribture `schema` will simply get ignored and
hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting
overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific
params? Also, all other superclass shouldn't worry about `self.schema =
<schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute.
I'm not sure I understand the purpose of the class level `schema.? When we do
`self.schema = None` in subclass, the class attribture `schema` will simply get
ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
> self.schema = None in its __init__ without worrying the value getting
overridden by super().__init__.
Hmm, shouldn't `super().__init__` being ran before other subclass-specific
params? Also, all other superclass shouldn't worry about `self.schema =
<schema>` because it should be captured by `super().__init__()`
```python
def __init__(self, *args, **kwargs):
super.__init__(*args, **kwargs) # `schema` should go in here
```
I think it's rather confusing to mix class attribute and instance attribute.
I'm not sure I understand the purpose of the class level `schema`. When we do
`self.schema = None` in subclass, the class attribture `schema` will simply get
ignored and hidden by the instance attibute `self.schema`. i.e.
```python
class Parent:
schema = None
class Child(Parent):
def __init__(self, schema):
self.schema = schema
child = Child('postgres')
print(child.__class__.schema)
print(child.schema)
print(Child.schema)
print(Parent.schema)
# None
# postgres
# None
# None
```
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to
`DbApiHook` and pass it through the `__init__` call, so all subclasses can all
inherit this properly.
##########
File path: airflow/hooks/dbapi.py
##########
@@ -53,6 +53,8 @@ class DbApiHook(BaseHook):
supports_autocommit = False
# Override with the object that exposes the connect method
connector = None # type: Optional[ConnectorProtocol]
+ # Override to use different schema from connection setting
+ schema = None
Review comment:
Ahh, no lol. Yeah, I'm suggesting adding it as an argument to
`DbApiHook` and pass it through the `__init__` call, so all subclasses can
inherit this properly.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
