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 2022/01/04 13:07:20 UTC
[GitHub] [airflow] subkanthi opened a new pull request #20651: Fix mypy in providers/grpc and providers/imap
subkanthi opened a new pull request #20651:
URL: https://github.com/apache/airflow/pull/20651
Related: #19891
Small change
Fixes the following mypy errors:
```
airflow/providers/imap/hooks/imap.py:74: error: Module has no attribute "IMAP4_SSL_PORT"; maybe "IMAP4_SSL"?
self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
airflow/providers/grpc/operators/grpc.py:90: error: Argument 1 of "execute" is incompatible with supertype "BaseOperator"; supertype defines the argument type as
"Context"
def execute(self, context: Dict) -> None:
```
<!--
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/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#issuecomment-1006438600
I merge this one. The fail was because of another PR merged to early and failing main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778221504
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
Yeap, if we do not pass the port in the constructor then the default will be used.
I even wanted to suggest `__all__.extend("IMAP4_SSL", "IMAP4_SSL_PORT"])` here
https://github.com/python/cpython/blob/7d7817cf0f826e566d8370a0e974bbfed6611d91/Lib/imaplib.py#L1338
but decided that these changes won't be applied soon
Do not close :)
Let's just wait for the review and will see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] subkanthi commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r779256708
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
Thanks, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] subkanthi commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778215558
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
I can close this PR, this was supposed to be minor changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#issuecomment-1004945571
Ooops. I've also worked on gRPC and IMAP in https://github.com/apache/airflow/pull/20654
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] subkanthi commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778214739
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
I was going off this, the default value for port in the constructor for IMAP4_SSL
```
class IMAP4_SSL(IMAP4):
"""IMAP4 client class over SSL connection
Instantiate with: IMAP4_SSL([host[, port[, keyfile[, certfile[, ssl_context]]]]])
host - host's name (default: localhost);
port - port number (default: standard IMAP4 SSL port);
keyfile - PEM formatted file that contains your private key (default: None);
certfile - PEM formatted certificate chain file (default: None);
ssl_context - a SSLContext object that contains your certificate chain
and private key (default: None)
Note: if ssl_context is provided, then parameters keyfile or
certfile should not be set otherwise ValueError is raised.
for more documentation see the docstring of the parent class IMAP4.
"""
def __init__(self, host='', port=IMAP4_SSL_PORT, keyfile=None,
certfile=None, ssl_context=None):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#issuecomment-1006241872
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20651:
URL: https://github.com/apache/airflow/pull/20651
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r779248354
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
Looks like a good alternative solution to me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r779227978
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
My solution is
```
conn = self.get_connection(self.imap_conn_id)
if conn.port:
self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port)
else:
self.mail_client = imaplib.IMAP4_SSL(conn.host)
self.mail_client.login(conn.login, conn.password)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy edited a comment on pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy edited a comment on pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#issuecomment-1004945571
Ooops. I've also worked on gRPC and IMAP in https://github.com/apache/airflow/pull/20654
But if you insist I can remove these providers` changes from my PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778212808
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
I think this solution might be not correct.
There is no class attribute for `IMAP4_SSL`, and `self.port` is defined in method `open`.
See how I decided to resolve this issue
https://github.com/apache/airflow/pull/20654/files#diff-b986a5d7078309e7eaad82d9b534ead3b21f0f884c927fdd21674cd4390a935a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778221504
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
Yeap, if we do not pass the port in the constructor then the default will be used.
I even wanted to suggest `__all__.extend("IMAP4_SSL", "IMAP4_SSL_PORT"])` here
https://github.com/python/cpython/blob/7d7817cf0f826e566d8370a0e974bbfed6611d91/Lib/imaplib.py#L1338
but decided that these changes won't be applied soon
Let's just wait for the review and will see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778589556
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
I don’t think `IMAP4_SSL.port` is going to work. Whether `IMAP4_SSL_PORT` is public or not is slightly ambiguous; it is not by module definition, [but the variable actually appears in documentation](https://docs.python.org/3/library/imaplib.html#imaplib.IMAP4_SSL). Either case, the IMAP-over-SSL port is standardised (993), so maybe we should just not rely on `imaplib.IMAP4_SSL_PORT` and just define our own variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#issuecomment-1006018871
Hi @subkanthi
I've removed conflicting changes from my PR #20654.
So go ahead :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #20651: Fix mypy in providers/grpc and providers/imap
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #20651:
URL: https://github.com/apache/airflow/pull/20651#discussion_r778661904
##########
File path: airflow/providers/imap/hooks/imap.py
##########
@@ -71,7 +71,7 @@ def get_conn(self) -> 'ImapHook':
"""
if not self.mail_client:
conn = self.get_connection(self.imap_conn_id)
- self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL_PORT)
+ self.mail_client = imaplib.IMAP4_SSL(conn.host, conn.port or imaplib.IMAP4_SSL.port)
Review comment:
The `IMAP4_SSL_PORT` is not public, but it is a default port when the only the host is passing to the constructor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org