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.

Sign up for GitHub

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

Jump to bottom

Fix order of operators before executing the git command #431

Merged
merged 1 commit into from
May 19, 2016
Merged

Fix order of operators before executing the git command #431

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions git/cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import errno
import mmap

from collections import OrderedDict
Copy link
Contributor

Choose a reason for hiding this comment

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

This change causes GitPython to be no longer compatible with Python 2.6, even if https://pypi.python.org/pypi/ordereddict is installed...

Is that the intention, or should this be replaced with:

from git.odict import OrderedDict

as is done here: https://github.com/gitpython-developers/GitPython/blob/master/git/config.py#L20 ?

I'm happy to issue a pull request for this, as I would really like GitPython to remain compatible with Python 2.6 for now...

Should .travis.yml be updated as well to test against Python 2.6 + ordereddict?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @boegel,

Indeed, with release 2.0 we officially dropped support for py2.6, assuming that this move is fine as py2.6 is not maintained anymore.

Nonetheless, I see no harm in using the alternate odict implementation, and would very much welcome a PR.

Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Python 2.6 is still the default Python in several operating systems, for example anything Red Hat 6.x based (CentOS 6, RHEL 6, Scientific Linux 6, etc.).

But, I understand the move, the only way is forward, I guess we should just stick to the latest GitPython 1.x then on these systems.

Would you mind sharing a pointer to the official announcement that mentions dropping Py2.6 support?

Copy link
Member

Choose a reason for hiding this comment

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

Of course: You will find these announcements in the changelog.
The cause of the move was some minor detail (which I forgot), so I believe after fixing this issue, 2.0 has a good change to run in py2.6 anyway. It's worth a try, considering how trivial the odict fix seems to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Byron I agree it's worth fixing, especially since git.odict is still there.

Should I also look into adding (back) testing for Py2.6 in .travis.yml, indicating that the tests for Py2.6 are allowed to fail? I feel that may help in at least knowing that Py2.6 compatibility is broken.

Copy link
Member

Choose a reason for hiding this comment

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

I very much like the idea - a commit is on the way to see what happens.

Copy link
Member

Choose a reason for hiding this comment

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

Here is the result: https://travis-ci.org/gitpython-developers/GitPython/jobs/133041770 .

Seems to be just one part of the compat code, which may be fixable in some way in case the blame functionality is important to someone.

Copy link
Contributor

Choose a reason for hiding this comment

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

for future reference: this issue got fixed in #431, but other incompatibilities with Py2.6 remain, e.g. https://travis-ci.org/gitpython-developers/GitPython/jobs/133041770

I won't be spending time on those (for now), I'll just stick with the latest GitPython 1.x on Py2.6 (cfr. easybuilders/easybuild-framework#1781)


from contextlib import contextmanager
import signal
from subprocess import (
Expand Down Expand Up @@ -783,6 +785,7 @@ def transform_kwarg(self, name, value, split_single_char_options):
def transform_kwargs(self, split_single_char_options=True, **kwargs):
"""Transforms Python style kwargs into git command line options."""
args = list()
kwargs = OrderedDict(sorted(kwargs.items(), key=lambda x: x[0]))
Copy link

Choose a reason for hiding this comment

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

nitpick: you could use operator.itemgetter() for more readability

for k, v in kwargs.items():
if isinstance(v, (list, tuple)):
for value in v:
Expand Down