FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix order of operators before executing the git command by guyzmo · Pull Request #431 · gitpython-developers/GitPython · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Fix order of operators before executing the git command #431

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

Merged
Byron merged 1 commit into from
May 19, 2016
Conversation 14 Commits 1 Checks 0 Files changed
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 .py  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
3 changes: 3 additions & 0 deletions git/cmd.py
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import errno
import mmap

from collections import OrderedDict

boegel May 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Byron May 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

boegel May 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Byron May 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

boegel May 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Byron May 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Byron May 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

boegel May 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dmerejkowsky May 12, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL