uranusjr commented on a change in pull request #15409:
URL: https://github.com/apache/airflow/pull/15409#discussion_r616309811
##########
File path: airflow/providers/sftp/hooks/sftp.py
##########
@@ -198,7 +205,9 @@ def delete_directory(self, path: str) -> None:
conn = self.get_conn()
conn.rmdir(path)
- def retrieve_file(self, remote_full_path: str, local_full_path: str) ->
None:
+ def retrieve_file(
+ self, remote_full_path: str, local_full_path: str, regex_pattern:
Optional[str] = None
+ ) -> None:
Review comment:
One thing I really dislike about this implementation is it subtly
changes semantics depending on the arguments. In this function, if
`regex_pattern` is None, `remote_full_path` should point to a file, but when
pattern matching is performed, it should point to a directory instead (and the
file name matched with the pattern). This will be very unintuitive in practice
and likely cause headaches to both users and maintainers.
Pattern matching is definitely a useful feature here, but I really don’t
think this is how it should be done.
--
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:
[email protected]
