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

Remove Python 2 compatibility shims #979

Merged
merged 32 commits into from
Feb 8, 2020

Conversation

Harmon758
Copy link
Member

@Harmon758 Harmon758 commented Feb 7, 2020

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.

@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #979 into master will increase coverage by 0.92%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #979      +/-   ##
==========================================
+ Coverage   93.35%   94.27%   +0.92%     
==========================================
  Files          59       59              
  Lines        9715     9540     -175     
==========================================
- Hits         9069     8994      -75     
+ Misses        646      546     -100
Impacted Files Coverage Δ
git/objects/tree.py 58.33% <100%> (-0.74%) ⬇️
git/test/test_index.py 96.18% <100%> (-0.01%) ⬇️
git/test/test_submodule.py 97.94% <100%> (ø) ⬆️
git/test/test_repo.py 96.78% <100%> (+0.27%) ⬆️
git/test/test_config.py 98.78% <100%> (-0.01%) ⬇️
git/objects/commit.py 91.13% <100%> (-0.04%) ⬇️
git/diff.py 98.77% <100%> (+0.77%) ⬆️
git/test/test_git.py 97.63% <100%> (+1.1%) ⬆️
git/refs/symbolic.py 96.11% <100%> (ø) ⬆️
git/objects/submodule/base.py 92.89% <100%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e27496...c5f5911. Read the comment docs.

@Byron Byron self-requested a review February 8, 2020 02:42
Copy link
Member

@Byron Byron left a comment

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.

@Byron Byron added this to the v3.0.6 - Bugfixes milestone Feb 8, 2020
@Byron Byron merged commit ff4f970 into gitpython-developers:master Feb 8, 2020
@Byron
Copy link
Member

Byron commented Feb 8, 2020

Released https://pypi.org/project/GitPython/3.0.7/

@Byron
Copy link
Member

Byron commented Feb 8, 2020

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.

@Harmon758
Copy link
Member Author

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.

@Harmon758 Harmon758 deleted the python-2-support-removal branch February 8, 2020 04:30
@Byron
Copy link
Member

Byron commented Feb 8, 2020

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.

@Byron
Copy link
Member

Byron commented Feb 8, 2020

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

@Harmon758
Copy link
Member Author

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?

@Byron
Copy link
Member

Byron commented Feb 8, 2020

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants