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