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

Fixes #1361 and #1027 - update to python 3.2+ API (existing code calls removed API) #1365

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

ms-jpq
Copy link
Contributor

@ms-jpq ms-jpq commented Feb 16, 2024

Fixes:

Should be OK to replace the removed API call, since the replacement was added in python 3.2.

https://docs.python.org/3/library/configparser.html

@TurtleWilly
Copy link

Should be OK to replace the removed API call, since the replacement was added in python 3.2.

Wouldn't that break Python 2.x support?

I still see tons of legacy Python 2 fallback code all over the place in s3cmd's code. IMHO removing Python 2 support should be done in a more organized fashion (aka all at once, then bump the major version) rather than a tidbit here or there and breaking things for people that still expect Python 2 support to work as the documentation suggests: "S3cmd requires Python 2.6 or newer." (quote from master readme.md). [Note: I use 3.12, but I know how it is when projects/ packages break backwards compatibility for no particular reasons. ]

@lavigne958
Copy link
Contributor

Should be OK to replace the removed API call, since the replacement was added in python 3.2.

Wouldn't that break Python 2.x support?

I still see tons of legacy Python 2 fallback code all over the place in s3cmd's code. IMHO removing Python 2 support should be done in a more organized fashion (aka all at once, then bump the major version) rather than a tidbit here or there and breaking things for people that still expect Python 2 support to work as the documentation suggests: "S3cmd requires Python 2.6 or newer." (quote from master readme.md). [Note: I use 3.12, but I know how it is when projects/ packages break backwards compatibility for no particular reasons. ]

I agree with you this will break code for people using python2.X

though if nothing is done, it will break code for people using python3.12+ so either end it will break someone's code.

So far python2.7 has been deprecated in 2008, and reached its end-of-life in 2020. I think it's time to move forward.
Users with the strong need to support python2 can stay on current release of s3cmd.

I believe things should be done in this order:

  1. remove the mention "requires python2.6" from the README
  2. update the code to use config.read_file()

Later on more changes could be added to remove python2 support.

@fviard
Copy link
Contributor

fviard commented Jan 20, 2025

Thank you for your contributions.
Sorry for the very long feedback, I would prefer to still not break python 2.x support when backward compatibility is still not difficult to preserve.

In the current case, it would be easy to fix like it is done in other files:

  • Adding :
    PY3 = (sys.version_info >= (3, 0)) at the top of this source file.
  • Using a :
    if PY3:
          ...read_file...
    else:
           ...readfp...
    

Would it be possible for you to fix your PR with that? Otherwise, as I know that it has been a long time, I will do it :-)

@ms-jpq
Copy link
Contributor Author

ms-jpq commented Jan 20, 2025

Thank you for your contributions. Sorry for the very long feedback, I would prefer to still not break python 2.x support when backward compatibility is still not difficult to preserve.

In the current case, it would be easy to fix like it is done in other files:

* Adding :
  ` PY3 = (sys.version_info >= (3, 0))` at the top of this source file.

* Using a :
  ```
  if PY3:
        ...read_file...
  else:
         ...readfp...
  ```

Would it be possible for you to fix your PR with that? Otherwise, as I know that it has been a long time, I will do it :-)

thanks 👍

S3/Config.py Outdated Show resolved Hide resolved
@fviard fviard merged commit 1a793a9 into s3tools:master Jan 20, 2025
8 checks passed
@fviard
Copy link
Contributor

fviard commented Jan 20, 2025

Merged, thank you very much!

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.

4 participants