Remove Python 2 compatibility shims by Harmon758 · Pull Request #979 · gitpython-developers/GitPython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits32 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

3.0.0 was supposed to have removed Python 2 support, but didn't actually remove any of the compatibility shims for Python 2.

In fact, with 3b13c11 (#860), further shims were added for Python 2 and going so far as to add future as a dependency and check for Python <= 2.3 (when sys.getfilesystemencoding was added). I'm not sure why #860 was merged with those shims, especially since it was failing a test due to switching to Unicode strings in Python 2, but this commit that did so improperly merged requirements.txt as well, readding gitdb and ddt as dependencies when they had been removed with ce21f63 (#826), so that setup.py could use requirements.txt. gitdb itself had actually already been removed as a dependency over 2 years before requirements.txt was updated, with 2ca7c01 in v2.0.9.
The commit was then reverted with cc664f0, referencing failing Travis CI tests due to future having been added as a dependency in requirements.txt and the Travis CI configuration installing dependencies from test-requirements.txt, rather than requirements.txt and/or setup.py. The changes were then reintroduced with 74a0507, adding installation of requirements.txt (despite what the commit summary says) to the Travis CI configuration. It was then again reverted with c9dbaab, referencing the failing Travis CI build for the previous commit due to the same failing test for the PR.
Afterwards, for some reason, the changes were yet again reintroduced with dac619e, the very commit that dropped Python 2 support. This time, future wasn't even added to requirements.txt, so Python 2 would have failed to import the library even if the commit hadn't dropped support for it. The changes were again reverted with 913d806 (for the 3rd time) when Python 2 support was brought back after being dropped in a patch version. They were then introduced again (for the 4th time) with 2e7e82b and 3.0.0 was subsequently released with the readded dependencies from almost 3 years ago, leading to #908. #908 was closed with ca080bb in 3.0.1, only removing gitdb as a dependency, leading to #911. #911 was then merged with df5c941 and released as 3.0.2, resolving the dependency issues caused by the improper merge, but leaving the pointless and broken compatibility shims that caused this mess in place.

This PR removes those and other compatibility shims for Python 2 and old Python 3 versions:

Remove from builtins import str - This was never even in any release that supported Python 2 and no release ever included future as a dependency.
Remove check for the existence of sys.getfilesystemencoding - It has existed since Python 2.3 as mentioned earlier.
Remove and replace compat._unichr with chr, comapt.bchr with bytes([...]), compat.binary_type with bytes, compat.bytes_chr with bytes((...,)), compat.FileType with io.IOBase, compat.izip with zip, compat.MAXSIZE with sys.maxsize, compat.mviter with .values(), compat.string_types with str, compat.text_type with str, compat.unicode with str, compat.xrange with range, compat.UnicodeMixin subclass by using __str__ rather than __unicode__
Remove compat.byte_ord, compat.PY3, compat.range
Remove surrogateescape error handler for Python 2
Remove Python 2 checks for compat.defenc, TestFun.test_tree_entries_from_data_with_failing_name_decode_py3, TestIndex.test_add_utf8P_path
Remove Python 2 checks in Git.__unpack_args, compat.with_metaclass, Diff.__str__, TestRepo.test_untracked_files, Actor._main_actor
Remove Python 3 checks in GitConfigParser._read, _octal_repl, run_commit_hook, git.objects.tree, RefLogEntry.__repr__, Repo._get_untracked_files, TestGit.test_call_unpack_args_unicode, TestGit.test_call_unpack_args
Remove TestFun.test_tree_entries_from_data_with_failing_name_decode_py2 Python 2 test
Remove attempt to import ConfigParser for Python 2 in git.config
Remove Python 2.7 check in TestBase
Remove check for logging.NullHandler for Python 2.6 in git.util
Remove Python < 3.3 check for PermissionError in git.cmd

This PR also fixes the formatting of the version specifier in requirements.txt (so as not to be in parentheses as #826 changed it to be with ce21f63) and improves setup.py's python_requires so as to succinctly specify >=3.4.

Choose a reason for hiding this comment

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

I am thoroughly impressed by your detective work, and the time and energy you must have spent to make this PR happen 🙇🏽‍♂️. and can't thank you enough for that!

Reading the script to a drama I myself created clearly shows my lack of aptitude in maintaining this project, and only gives a hint on the negative impact this may have had on the users. Despite me trying my best, it's fair to apologize to those I may have put into harms way.

To me nothing of this comes at a surprise, after all I am not writing Python anymore, nor am I using GitPython. This is my call for help to prevent accidents like the above from happening in the future, because I can't make any promises while I am the sole maintainer. No, I even fear it may get worse.

This PR is a step in the right direction - it removes code instead of adding it, and will hopefully be the first of more to come.
In the meantime, any help in reviewing PRs is appreciated, and if you would like to become a maintainer, please let me know 🙏.

Please expect a new release to happen soon.

When merging, for some reason, I see this test breaking. I am certain it's related to the test suite not being properly isolated, but hope that given your prior GitPython experience, this makes more sense to you.

If you cannot help, please let me know and I will dig in.

Thanks for the merge and release 🎉
I wouldn't mind helping to review PRs and/or becoming a maintainer.
In the latter case, it might take me a bit to get to know all of the code base, and I'd probably still make PRs for more significant changes, but I actually did have some other minor updates that I'm looking into.

When merging, for some reason, I see this test breaking. I am certain it's related to the test suite not being properly isolated, but hope that given your prior GitPython experience, this makes more sense to you.

Odd, this seems to be a transient issue with the number of reference types for the repo.
I'll look into it, but it seems it hasn't reoccurred yet? At any rate, I don't think it's related to this PR.

Fantastic news! You should receive an invite momentarily! And I just checked travis, it's all green now. Let's ignore it, I am sure there is more interesting things to do than chasing phantoms :D.

Please take your time to get acquainted, I am happy about any help no matter how small. You are entirely unconstrained as I trust you know what you are doing, so I will never say 'no', but am happy to share my opinion. Once you need a release, just ping me and I will do it with low delay.

Lastly, generally I work off some emails once a month or so (or if there are too many piling up), giving preference to PRs. The more elaborate the PRs, the more likely I will respond quickly (as you might have noticed). This also gives you ample time to pick up PRs or issues that arrive in your inbox, before I do.

In GitPython, I think it's safe to receive notifications for all issues and PR, the volume is quite low with less than 10 notifications per month.

And if there are any questions, or anything really, please reach out right away.

Thanks, and have fun :)

Thanks! Happy to join the team.

Let's ignore it, I am sure there is more interesting things to do than chasing phantoms :D.

Yeah, I'll look into it further if it reoccurs.

You are entirely unconstrained as I trust you know what you are doing, so I will never say know, but am happy to share my opinion.

I generally make branches and/or PRs for substantial changes so that they can be reviewed and/or tested, so I'll probably request reviews for those.

This also gives you ample time to pick up PRs or issues that arrive in your inbox, before I do.

I do maintain and help with other libraries, but I'll try to triage when I have time.

In GitPython, I think it's safe to receive notifications for all issues and PR, the volume is quite low with less than 10 notifications per month.

I'm already watching the repo now 👍

Once you need a release, just ping me and I will do it with low delay.

And if there are any questions, or anything really, please reach out right away.

Is there a good place to contact you besides GitHub? e.g. the email in your commits, Keybase chat, or?

Probably easiest is to use the (brand new) gitter room: https://gitter.im/gitpython-developers/GitPython . I wouldn't want to advertise it to avoid chatter, so right now it's really just you and me there. Otherwise, the email address in my recent commits will work, too.

This was referenced

Feb 16, 2020

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request

Jan 23, 2024

The NullHandler class in git.util was added when merging gitpython-developers#300, to allow a noop handler to be used on Python 2.6, since the standard library logging.NullHandler class was added in Python 2.7.

When introduced in d1a9a23, the git.util.NullHandler class was also patched into the logging module, but that has no longer been done since 2fced2e (gitpython-developers#979), nor does GitPython make other use of it.

This also changes the parameter type annotation on the emit method from object to logging.LogRecord, which is the expeced type.