You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/03/27 11:06:58 UTC
[GitHub] [airflow] zhongjiajie opened a new pull request #7903: Add property
conn_name to dbapi_hook
zhongjiajie opened a new pull request #7903: Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399219644
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
I change this un related cause it take me a little time to get the point, I think we should use `if condition ... else if ... else <default_value>` for more straightforward.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on issue #7903: [dont-merge] Add
property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-604942825
We have one subclass hook still WIP in https://github.com/apache/airflow/pull/7901
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399329884
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
```python
else:
setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name)
```
instead of
```python
elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
else:
setattr(self, self.conn_name_attr, self.default_conn_name)
```
WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on issue #7903: Add conn_name and
get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-610706420
And for now, the solution two pass CI, and I think it work.
The question is, should we take solution one and change `get_connection` in `dbapi_hook` to property `connection`? If we use solution one, subclass of `dbapi_hook` have to use `self.connection` to get connection which is different from subclass from `base_hook`(using `get_connection`)
But if we take solution two, `get_connection` don't pass `mypy` check, and `self.get_connection` and `DbApiHook.get_connection` call different method, I think it will confuse user when calling 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399354993
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
Oh, that much simple, but I have half-memories use `dict[key]` for exactly key as best practices. WDYT.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add
conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r400906384
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,32 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
+ """
+ return getattr(self, self.conn_name_attr)
+
+ def get_connection(self, conn_id: Optional[str] = None) -> Connection:
+ """
+ Get connection from conn_id, if conn_id is None,
+ will get from `getattr(self, self.conn_name_attr)`
Review comment:
```suggestion
Get the database connection object.
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399355323
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
You're right
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
Add conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r404662162
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,31 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get the name of the database connection attribute which identifies the connection.
+ """
+ return getattr(self, self.conn_name_attr)
+
+ def get_connection(self, conn_id: Optional[str] = None) -> Connection:
Review comment:
Emmy, I think we get some problem here, `mypy` unhappy and said `76: error: Signature of "get_connection" incompatible with supertype "BaseHook"`, because in `BaseHook` function `get_connection` is `classmethod`, but in `DbApiHook` function `get_connection` we use `getattr(self, self.conn_name_attr)` so have to be instance method.
And `DbApiHook` we do not need to pass `conn_id` anymore, and I think we should change function `get_connection` to `@property connection` here. WDYT
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on issue #7903: Add conn_name and
get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-606709915
My laptop almost power off and I forget the charger in office, have to continue in tomorrow.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on issue #7903: Add conn_name and
get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-606360029
For reviewer, in https://github.com/apache/airflow/pull/7903/commits/cf0a5f6fca8f219d0038c4c7ace5d98695400ce4 I just add property `conn_name` to get `conn_name`. And then I find out we could change `conn = self.get_connection(self.conn_name)` to `conn = self.get_connection()` due to `self.conn_name` almost same in subclass of `DBAPI` and is no need, So I add function `get_connection` in `dbapi_hook` to get conn_name if no conn_id pass from subclass.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399354993
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
Oh, that much simple, but I have half-memories use `dict[key]` for exactly key as best practices. WDYT.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on issue #7903: Add conn_name and
get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-606625417
> It looks good. I would like to understand though why it was done like that in the first place. seems unnecessary complex - maybe others (@feluelle @mik-laj can take a look and shed some light on it ) :)
I agree with you Jarek. I also don't know why it is as it is. 🤔
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r400603652
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
Add 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie edited a comment on issue #7903: Add
conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie edited a comment on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-606360029
For reviewer, in https://github.com/apache/airflow/pull/7903/commits/cf0a5f6fca8f219d0038c4c7ace5d98695400ce4 I just add property `conn_name` to get `conn_name`.
And then I find out we could change `conn = self.get_connection(self.conn_name)` to `conn = self.get_connection()` due to `self.conn_name` almost same in subclass of `DBAPI` and is no need, So I add function `get_connection` in `dbapi_hook` to get conn_name if no conn_id pass from subclass. In the last change in https://github.com/apache/airflow/pull/7903/commits/e5aa2a4b4d79db3d8aec7d6ad1272d519952da84
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #7903: Add conn_name and
get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#issuecomment-610487037
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=h1) Report
> Merging [#7903](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `55.04%`.
> The diff coverage is `11.42%`.
[![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7903/graphs/tree.svg?width=650&height=150&src=pr&token=WdLKlKHOAU)](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7903 +/- ##
===========================================
- Coverage 88.31% 33.26% -55.05%
===========================================
Files 935 935
Lines 45170 45170
===========================================
- Hits 39892 15027 -24865
- Misses 5278 30143 +24865
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/providers/apache/druid/hooks/druid.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2RydWlkL2hvb2tzL2RydWlkLnB5) | `24.35% <0.00%> (-65.39%)` | :arrow_down: |
| [airflow/providers/apache/hive/hooks/hive.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvaG9va3MvaGl2ZS5weQ==) | `13.88% <0.00%> (-63.64%)` | :arrow_down: |
| [airflow/providers/apache/pinot/hooks/pinot.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL3Bpbm90L2hvb2tzL3Bpbm90LnB5) | `19.23% <0.00%> (-72.31%)` | :arrow_down: |
| [...low/providers/elasticsearch/hooks/elasticsearch.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZWxhc3RpY3NlYXJjaC9ob29rcy9lbGFzdGljc2VhcmNoLnB5) | `21.62% <0.00%> (-24.54%)` | :arrow_down: |
| [airflow/providers/jdbc/hooks/jdbc.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvamRiYy9ob29rcy9qZGJjLnB5) | `47.36% <0.00%> (-52.64%)` | :arrow_down: |
| [airflow/providers/microsoft/mssql/hooks/mssql.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbWljcm9zb2Z0L21zc3FsL2hvb2tzL21zc3FsLnB5) | `61.90% <0.00%> (-14.29%)` | :arrow_down: |
| [...rflow/providers/microsoft/mssql/operators/mssql.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbWljcm9zb2Z0L21zc3FsL29wZXJhdG9ycy9tc3NxbC5weQ==) | `42.42% <0.00%> (-48.21%)` | :arrow_down: |
| [airflow/providers/mysql/hooks/mysql.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvaG9va3MvbXlzcWwucHk=) | `19.79% <0.00%> (-72.92%)` | :arrow_down: |
| [airflow/providers/odbc/hooks/odbc.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvb2RiYy9ob29rcy9vZGJjLnB5) | `25.55% <0.00%> (-66.67%)` | :arrow_down: |
| [airflow/providers/oracle/hooks/oracle.py](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvb3JhY2xlL2hvb2tzL29yYWNsZS5weQ==) | `9.40% <0.00%> (-86.33%)` | :arrow_down: |
| ... and [822 more](https://codecov.io/gh/apache/airflow/pull/7903/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=footer). Last update [0dafdd0...124c19d](https://codecov.io/gh/apache/airflow/pull/7903?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399355984
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
Review comment:
Oh, that much simple, but I have half-memories use `dict[key]` for exactly key as best practices. WDYT.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add
conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r400904172
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,32 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
+ """
+ return getattr(self, self.conn_name_attr)
+
+ def get_connection(self, conn_id: Optional[str] = None) -> Connection:
+ """
+ Get connection from conn_id, if conn_id is None,
+ will get from `getattr(self, self.conn_name_attr)`
+
+ :param conn_id: connection id
+ :type conn_id: Optional[str]
+ :return: Connection
+ """
+ conn_name = conn_id if conn_id is not None else self.conn_name
Review comment:
```suggestion
conn_name = conn_id or self.conn_name
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add
conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r400905636
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
You missed that.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399325848
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
We don't add production code only because a linter is unhappy!
What it give us is that it..
```suggestion
Get the name of the database connection attribute which identifies the connection.
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399356160
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
You're right.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
Add conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r401018865
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
Sorry, I just force push and forget fetch from my origin
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
[dont-merge] Add property conn_name to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: [dont-merge] Add property conn_name to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r399355323
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -61,10 +61,17 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
- else:
+ elif self.conn_name_attr in kwargs:
setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ else:
+ setattr(self, self.conn_name_attr, self.default_conn_name)
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get conn_name to avoid pylint no-member in subclass
Review comment:
You're right
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903:
Add conn_name and get_connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903: Add conn_name and get_connection to dbapi_hook
URL: https://github.com/apache/airflow/pull/7903#discussion_r404695613
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,31 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get the name of the database connection attribute which identifies the connection.
+ """
+ return getattr(self, self.conn_name_attr)
+
+ def get_connection(self, conn_id: Optional[str] = None) -> Connection:
Review comment:
Or maybe we could just use `# type: ignore` to ignore `mypy` in L76
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services