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/09/27 00:30:17 UTC
[GitHub] [airflow] mlgruby opened a new pull request #11170: Increasing type coverage for five different providers
mlgruby opened a new pull request #11170:
URL: https://github.com/apache/airflow/pull/11170
Increasing type coverage for multiple providers. Part of: #9708
cc: @kaxil @mik-laj another type coverage one, can I request for review of this PR?
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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).
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495576366
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -73,7 +72,9 @@ def is_successful(self) -> bool:
"""True if the result state is SUCCESS"""
return self.result_state == 'SUCCESS'
- def __eq__(self, other) -> bool:
+ def __eq__(self, other: object) -> bool:
+ if not isinstance(other, RunState):
+ return NotImplemented
Review comment:
Should it be raise?
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495595647
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -325,9 +331,9 @@ class _TokenAuth(AuthBase):
magic function.
"""
- def __init__(self, token):
+ def __init__(self, token: str) -> None:
self.token = token
- def __call__(self, r):
+ def __call__(self, r: PreparedRequest) -> PreparedRequest:
Review comment:
you can find more info here: https://requests.readthedocs.io/en/master/api/#requests.PreparedRequest
If I am not wrong your request here it to provide a link in the comment (here) but not in the code!
##########
File path: airflow/providers/databricks/operators/databricks.py
##########
@@ -250,23 +251,23 @@ class DatabricksSubmitRunOperator(BaseOperator):
def __init__(
self,
*,
- json=None,
- spark_jar_task=None,
- notebook_task=None,
- spark_python_task=None,
- spark_submit_task=None,
- new_cluster=None,
- existing_cluster_id=None,
- libraries=None,
- run_name=None,
- timeout_seconds=None,
- databricks_conn_id='databricks_default',
- polling_period_seconds=30,
- databricks_retry_limit=3,
- databricks_retry_delay=1,
- do_xcom_push=False,
+ json: Optional[Any] = None,
Review comment:
tried that but below assignment is making mypy check a fail.
```python
self.json = _deep_string_coerce(self.json) #line 301
```
`_deep_string_coerce` return type is `Union[str, list, dict]` is not matching with the only 'dict' of JSON
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495596033
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -202,15 +202,17 @@ def get_conn(self) -> pyodbc.Connection:
conn = pyodbc.connect(self.odbc_connection_string, **self.connect_kwargs)
return conn
- def get_uri(self):
+ def get_uri(self) -> str:
"""
URI invoked in :py:meth:`~airflow.hooks.dbapi_hook.DbApiHook.get_sqlalchemy_engine` method
"""
quoted_conn_str = quote_plus(self.odbc_connection_string)
uri = f"{self.sqlalchemy_scheme}:///?odbc_connect={quoted_conn_str}"
return uri
- def get_sqlalchemy_connection(self, connect_kwargs=None, engine_kwargs=None):
+ def get_sqlalchemy_connection(
+ self, connect_kwargs: Optional[dict] = None, engine_kwargs: Optional[dict] = None
+ ) -> Any:
Review comment:
it returns an authorization connection object
in dbapi_hook.py the definition of connection function is as below:
```python
def connect(self, host: str, port: int, username: str, schema: str) -> Any:
```
hence used `Any` here
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#issuecomment-699638847
@XD-DENG Yup, make sense. Added more strict type now!
----------------------------------------------------------------
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
[GitHub] [airflow] XD-DENG merged pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #11170:
URL: https://github.com/apache/airflow/pull/11170
----------------------------------------------------------------
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
[GitHub] [airflow] XD-DENG commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495588989
##########
File path: airflow/providers/databricks/operators/databricks.py
##########
@@ -250,23 +251,23 @@ class DatabricksSubmitRunOperator(BaseOperator):
def __init__(
self,
*,
- json=None,
- spark_jar_task=None,
- notebook_task=None,
- spark_python_task=None,
- spark_submit_task=None,
- new_cluster=None,
- existing_cluster_id=None,
- libraries=None,
- run_name=None,
- timeout_seconds=None,
- databricks_conn_id='databricks_default',
- polling_period_seconds=30,
- databricks_retry_limit=3,
- databricks_retry_delay=1,
- do_xcom_push=False,
+ json: Optional[Any] = None,
Review comment:
Should this be `Optional[Dict]`?
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -202,15 +202,17 @@ def get_conn(self) -> pyodbc.Connection:
conn = pyodbc.connect(self.odbc_connection_string, **self.connect_kwargs)
return conn
- def get_uri(self):
+ def get_uri(self) -> str:
"""
URI invoked in :py:meth:`~airflow.hooks.dbapi_hook.DbApiHook.get_sqlalchemy_engine` method
"""
quoted_conn_str = quote_plus(self.odbc_connection_string)
uri = f"{self.sqlalchemy_scheme}:///?odbc_connect={quoted_conn_str}"
return uri
- def get_sqlalchemy_connection(self, connect_kwargs=None, engine_kwargs=None):
+ def get_sqlalchemy_connection(
+ self, connect_kwargs: Optional[dict] = None, engine_kwargs: Optional[dict] = None
+ ) -> Any:
Review comment:
Is this returning a more specific type?
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -325,9 +331,9 @@ class _TokenAuth(AuthBase):
magic function.
"""
- def __init__(self, token):
+ def __init__(self, token: str) -> None:
self.token = token
- def __call__(self, r):
+ def __call__(self, r: PreparedRequest) -> PreparedRequest:
Review comment:
May you provide a bit more info about this type? Say any link reference to requests doc relating to this? Thanks.
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#issuecomment-699565353
0.000000 dingding
0.000000 opsgenie
0.000000 presto
0.000000 qubole
0.000000 samba
0.000000 zendesk
16.666650 ssh
16.666667 oracle
16.666667 yandex
41.074865 amazon
52.078754 microsoft
69.100860 google
75.000000 postgres
77.619050 databricks ===============> from 23 to 77
77.678575 slack
80.625000 ftp
82.449500 cncf
83.333300 odbc ===================> from 16 to 83
83.333300 openfaas
87.421875 apache
87.500000 redis
88.333340 mysql
88.888900 papermill
90.909100 elasticsearch
96.428575 salesforce
96.875000 mongo
98.076900 imap
100.000000 celery
100.000000 cloudant
100.000000 datadog
100.000000 discord
100.000000 docker
100.000000 exasol ================> from 25 to 100
100.000000 facebook
100.000000 grpc
100.000000 hashicorp
100.000000 http
100.000000 jdbc ==================> from 75 to 100
100.000000 jenkins
100.000000 jira
100.000000 pagerduty
100.000000 plexus
100.000000 segment
100.000000 sendgrid
100.000000 sftp
100.000000 singularity =============> from 25 to 100
100.000000 snowflake
100.000000 sqlite
100.000000 vertica
----------------------------------------------------------------
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
[GitHub] [airflow] XD-DENG commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495552458
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -208,10 +213,10 @@ def _do_api_call(self, endpoint_info, json):
attempt_num += 1
sleep(self.retry_delay)
- def _log_request_error(self, attempt_num, error):
+ def _log_request_error(self, attempt_num: Any, error: Any) -> None:
Review comment:
Again, adding `Any` here is causing an impression that we have higher type coverage, but doesn't really contribute real "value".
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -49,7 +50,7 @@ class RunState:
Utility class for the run state concept of Databricks runs.
"""
- def __init__(self, life_cycle_state, result_state, state_message):
+ def __init__(self, life_cycle_state: Any, result_state: Any, state_message: Any) -> None:
Review comment:
Change in this line is not making much sense to me (putting `Any` is not adding coverage).
If I'm not mistaken, all of them can be more strict type, `str`, rather than `Any`.
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -113,7 +118,7 @@ def __init__(
self.retry_delay = retry_delay
@staticmethod
- def _parse_host(host):
+ def _parse_host(host: Any) -> str:
Review comment:
The same issue here.
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495577049
##########
File path: airflow/providers/databricks/hooks/databricks.py
##########
@@ -73,7 +72,9 @@ def is_successful(self) -> bool:
"""True if the result state is SUCCESS"""
return self.result_state == 'SUCCESS'
- def __eq__(self, other) -> bool:
+ def __eq__(self, other: object) -> bool:
+ if not isinstance(other, RunState):
+ return NotImplemented
Review comment:
It's a special value, which should be returned by binary special methods, we could have a raise for `NotImplementedError`.
More info here: https://docs.python.org/3/library/constants.html#NotImplemented
----------------------------------------------------------------
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
[GitHub] [airflow] mlgruby commented on a change in pull request #11170: Increasing type coverage for five different providers
Posted by GitBox <gi...@apache.org>.
mlgruby commented on a change in pull request #11170:
URL: https://github.com/apache/airflow/pull/11170#discussion_r495596033
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -202,15 +202,17 @@ def get_conn(self) -> pyodbc.Connection:
conn = pyodbc.connect(self.odbc_connection_string, **self.connect_kwargs)
return conn
- def get_uri(self):
+ def get_uri(self) -> str:
"""
URI invoked in :py:meth:`~airflow.hooks.dbapi_hook.DbApiHook.get_sqlalchemy_engine` method
"""
quoted_conn_str = quote_plus(self.odbc_connection_string)
uri = f"{self.sqlalchemy_scheme}:///?odbc_connect={quoted_conn_str}"
return uri
- def get_sqlalchemy_connection(self, connect_kwargs=None, engine_kwargs=None):
+ def get_sqlalchemy_connection(
+ self, connect_kwargs: Optional[dict] = None, engine_kwargs: Optional[dict] = None
+ ) -> Any:
Review comment:
it returns an authorization connection object
in dbapi_hook.py the definition of connection function is as below which is a return for above function:
```python
def connect(self, host: str, port: int, username: str, schema: str) -> Any:
```
hence used `Any` here
----------------------------------------------------------------
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