[GitHub] [airflow] zeroam commented on a change in pull request #7953: close sftp connection without error
zeroam commented on a change in pull request #7953: close sftp connection without error URL: https://github.com/apache/airflow/pull/7953#discussion_r399871650 ## File path: airflow/providers/sftp/hooks/sftp.py ## @@ -120,12 +120,11 @@ def get_conn(self) -> pysftp.Connection: def close_conn(self) -> None: """ -Closes the connection. An error will occur if the -connection wasnt ever opened. +Closes the connection """ -conn = self.conn -conn.close() # type: ignore -self.conn = None +if self.conn is not None: +self.conn.close() +self.conn = None Review comment: I think when we initialize SFTPHook, self.conn is None. This may cause an error in two case 1. call `close_conn` without a connection 2. call `close_conn` more than once after connection it's not necessary to cause an error in this case 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] zeroam commented on a change in pull request #7953: close sftp connection without error
zeroam commented on a change in pull request #7953: close sftp connection without error URL: https://github.com/apache/airflow/pull/7953#discussion_r399870753 ## File path: airflow/providers/sftp/hooks/sftp.py ## @@ -120,12 +120,11 @@ def get_conn(self) -> pysftp.Connection: def close_conn(self) -> None: """ -Closes the connection. An error will occur if the -connection wasnt ever opened. +Closes the connection """ -conn = self.conn -conn.close() # type: ignore -self.conn = None +if self.conn is not None: Review comment: Good suggestion. I think `is not None` is more clear and cause less error. like this ```python >>> x = 0 >>> x is not None True >>> not x True >>> y = 1 >>> y is not None True >>> not y False >>> z = None >>> z is not None False >>> not z True ``` check this: https://stackoverflow.com/questions/2710940/python-if-x-is-not-none-or-if-not-x-is-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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
