Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Remove Python 2 compatibility shims#979

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

Already on GitHub?Sign in to your account

Merged

Conversation

Harmon758
Copy link
Member

@Harmon758Harmon758 commentedFeb 7, 2020
edited
Loading

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, with3b13c11 (#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 (whensys.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 withce21f63 (#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, with2ca7c01 in v2.0.9.
The commit was then reverted withcc664f0, 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 with74a0507, adding installation of requirements.txt (despite what the commit summary says) to the Travis CI configuration. It was then again reverted withc9dbaab, 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 withdac619e, 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 with913d806 (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) with2e7e82b and 3.0.0 was subsequently released with the readded dependencies from almost 3 years ago, leading to#908.#908 was closed withca080bb in 3.0.1, only removing gitdb as a dependency, leading to#911.#911 was then merged withdf5c941 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:

  • Removefrom 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 ofsys.getfilesystemencoding - It has existed since Python 2.3 as mentioned earlier.
  • Remove and replacecompat._unichr withchr,comapt.bchr withbytes([...]),compat.binary_type withbytes,compat.bytes_chr withbytes((...,)),compat.FileType withio.IOBase,compat.izip withzip,compat.MAXSIZE withsys.maxsize,compat.mviter with.values(),compat.string_types withstr,compat.text_type withstr,compat.unicode withstr,compat.xrange withrange,compat.UnicodeMixin subclass by using__str__ rather than__unicode__
  • Removecompat.byte_ord,compat.PY3,compat.range
  • Remove surrogateescape error handler for Python 2
  • Remove Python 2 checks forcompat.defenc,TestFun.test_tree_entries_from_data_with_failing_name_decode_py3,TestIndex.test_add_utf8P_path
  • Remove Python 2 checks inGit.__unpack_args,compat.with_metaclass,Diff.__str__,TestRepo.test_untracked_files,Actor._main_actor
  • Remove Python 3 checks inGitConfigParser._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
  • RemoveTestFun.test_tree_entries_from_data_with_failing_name_decode_py2 Python 2 test
  • Remove attempt to importConfigParser for Python 2 ingit.config
  • Remove Python 2.7 check inTestBase
  • Remove check forlogging.NullHandler for Python 2.6 ingit.util
  • Remove Python < 3.3 check forPermissionError ingit.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 withce21f63) and improves setup.py'spython_requires so as to succinctly specify >=3.4.

@codecov-io
Copy link

codecov-io commentedFeb 7, 2020
edited
Loading

Codecov Report

Merging#979 intomaster willincrease coverage by0.92%.
The diff coverage is97.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 FilesCoverage Δ
git/objects/tree.py58.33% <100%> (-0.74%)⬇️
git/test/test_index.py96.18% <100%> (-0.01%)⬇️
git/test/test_submodule.py97.94% <100%> (ø)⬆️
git/test/test_repo.py96.78% <100%> (+0.27%)⬆️
git/test/test_config.py98.78% <100%> (-0.01%)⬇️
git/objects/commit.py91.13% <100%> (-0.04%)⬇️
git/diff.py98.77% <100%> (+0.77%)⬆️
git/test/test_git.py97.63% <100%> (+1.1%)⬆️
git/refs/symbolic.py96.11% <100%> (ø)⬆️
git/objects/submodule/base.py92.89% <100%> (ø)⬆️
... and16 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update1e27496...c5f5911. Read thecomment docs.

@ByronByron self-requested a reviewFebruary 8, 2020 02:42
Copy link
Member

@ByronByron 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.

Harmon758 reacted with thumbs up emoji
@ByronByron added this to thev3.0.6 - Bugfixes milestoneFeb 8, 2020
@ByronByron merged commitff4f970 intogitpython-developers:masterFeb 8, 2020
@Byron
Copy link
Member

Releasedhttps://pypi.org/project/GitPython/3.0.7/

Harmon758 reacted with hooray emoji

@Byron
Copy link
Member

When merging, for some reason, I seethis 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
MemberAuthor

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 seethis 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.

Byron reacted with heart emoji

@Harmon758Harmon758 deleted the python-2-support-removal branchFebruary 8, 2020 04:30
@Byron
Copy link
Member

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 commentedFeb 8, 2020
edited
Loading

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
MemberAuthor

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 reacted with thumbs up emoji

@Byron
Copy link
Member

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 requestJan 23, 2024
The NullHandler class in git.util was added when merginggitpython-developers#300, toallow a noop handler to be used on Python 2.6, since the standardlibrary logging.NullHandler class was added in Python 2.7.When introduced ind1a9a23, the git.util.NullHandler class was alsopatched into the logging module, but that has no longer been donesince2fced2e (gitpython-developers#979), nor does GitPython make other use of it.This also changes the parameter type annotation on the emit methodfrom `object` to `logging.LogRecord`, which is the expeced type.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

Assignees
No one assigned
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
@Harmon758@codecov-io@Byron

[8]ページ先頭

©2009-2025 Movatter.jp