[GitHub] [airflow] potiuk commented on a change in pull request #15408: Add Connection Documentation to more Providers

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #15408:
URL: https://github.com/apache/airflow/pull/15408#discussion_r619688934



##########
File path: airflow/providers/sftp/hooks/sftp.py
##########
@@ -45,9 +45,12 @@ class SFTPHook(SSHHook):
           permissions.
 
     Errors that may occur throughout but should be handled downstream.
+
+    :param sftp_conn_id: The :ref:`sftp connection id<howto/connection:sftp>`
+    :type sftp_conn_id: str
     """
 
-    conn_name_attr = 'ftp_conn_id'
+    conn_name_attr = 'sftp_conn_id'

Review comment:
       This change is wrong.
   
   It should remain ftp_conn_id because this is the name of the constructor argument (below:
   ```
       def __init__(self, ftp_conn_id: str = 'sftp_default', *args, **kwargs) -> None:
   ```
   
   It is later remapped to "ssh_conn_id" just before running the constructor of the parent class, so it does not matter. Note that it is just a name used when the connection is instantiated dynamically so it does not really matter what it is is, as long as it is consistent. And if someone ever created the Hook manually with keyword argument, changing it would mean backwards incompatibility as mentioned by @uranusjr 




-- 
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