Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made encoding keyword backward compatible #8

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Made encoding keyword backward compatible #8

merged 4 commits into from
Nov 10, 2021

Conversation

richbon75
Copy link
Contributor

The encoding parameter for ftplib.FTP was added in Python 3.9.

The update to add the encoding parameter broke backward compatibility with versions earlier than 3.9 (they raise an unexpected keyword error).

This pull request modifies the code so that if the encoding parameter is not supported, it fails back to a version of the FTP constructor without the encoding parameter.

@mristin mristin changed the title make encoding keyword backward compatible Made encoding keyword backward compatible Nov 9, 2021
Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the suggested change.

reconnecting_ftp/__init__.py Outdated Show resolved Hide resolved
@mristin
Copy link
Collaborator

mristin commented Nov 9, 2021

Just before I squash and merge: @richbon75 did you test locally on your machine that it works as expected?

@richbon75
Copy link
Contributor Author

Yup. Seems to be working as expected.

@mristin
Copy link
Collaborator

mristin commented Nov 10, 2021

@richbon75 I just realized it might be very confusing for the user if s/he supplies the encoding parameter which is then ignored. I made yet another small suggestion.

Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor change slipped my attention -- please apologize the inconvenience; could you have another look?

@@ -112,7 +113,10 @@ def connect(self) -> None:
if self.connection is None:
conn_refused = None # type: Optional[ConnectionRefusedError]
try:
self.connection = self.FTP(timeout=self.timeout, encoding=self.encoding)
if sys.version_info >= (3, 9):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Unfortunately GitHub does not let me comment on unchanged lines. This comment refers to line 77.)

I only now realized that the user might be confused if the encoding parameter is simply ignored on older versions of Python.

What do you think about this additional change:

    def __init__(self,
             hostname: str,
             port: int,
             user: str,
             password: str,
             max_reconnects: int = 10,
             timeout: int = 10,
             FTP=ftplib.FTP,
             encoding: str = 'utf-8') -> None:
        # pylint: disable=too-many-arguments
        self.access = Access()
        self.access.hostname = hostname
        self.access.port = port
        self.access.user = user
        self.access.password = password
        self.connection = None  # type: Optional[ftplib.FTP]
        self.last_pwd = None  # type: Optional[str]
        self.encoding = encoding
        self.max_reconnects = max_reconnects
        self.timeout = timeout
        self.FTP = FTP  # pylint: disable=invalid-name
else:
    def __init__(self,
             hostname: str,
             port: int,
             user: str,
             password: str,
             max_reconnects: int = 10,
             timeout: int = 10,
             FTP=ftplib.FTP,
             encoding: str = 'utf-8') -> None:
        # pylint: disable=too-many-arguments
        self.access = Access()
        self.access.hostname = hostname
        self.access.port = port
        self.access.user = user
        self.access.password = password
        self.connection = None  # type: Optional[ftplib.FTP]
        self.last_pwd = None  # type: Optional[str]
        
        # No ``encoding `` is supported 
        # for this version of Python.
        
        self.max_reconnects = max_reconnects
        self.timeout = timeout
        self.FTP = FTP  # pylint: disable=invalid-name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite understand your intention in your code snippet, but I understood your suggestion that we not silently ignore a value provided for the encoding argument.

How about this latest update? It only raises an error IF a value is supplied for the encoding argument and the version < 3.9. If no argument is provided, the self.encoding attribute is never set. If the self.encoding attribute is not set, it is not passed to the call to self.FTP( ). This allows ftplib.FTP() to be responsible for setting the default value of encoding, and reconnecting-ftp doesn't have to provide a default at all.

@@ -82,8 +82,13 @@ def __init__(self,
max_reconnects: int = 10,
timeout: int = 10,
FTP=ftplib.FTP,
encoding: str = 'utf-8') -> None:
encoding: str = None) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer providing a default for encoding. If the user doesn't provide one, an explicit value is no longer sent, allowing ftplib.FTP to supply it's own default of 'utf-8' in versions 3.9+

Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a much better solution than mine!

@mristin mristin merged commit d6f5f62 into Parquery:master Nov 10, 2021
mristin added a commit that referenced this pull request Nov 10, 2021
* Made encoding keyword backward compatible (#8)
* Added badges and updated supported Python versions (#12)
@mristin mristin mentioned this pull request Nov 10, 2021
mristin added a commit that referenced this pull request Nov 10, 2021
* Made encoding keyword backward compatible (#8)
* Added badges and updated supported Python versions (#12)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants