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

avoid string version comparisons in external.qt #2831

Merged
merged 5 commits into from Jan 28, 2013

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Jan 22, 2013

We have learned from pyzmq that one should always parse version strings
before comparing them.

avoid string version comparisons in external.qt
We have learned from pyzmq that one should *always* parse version strings before comparing them.
@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 22, 2013

Member

LooseVersion is not necessarily comparable on Python 3 (whereas StrictVersion is). Something like V('1.0.dev') > V('1.0.3') will throw a TypeError. I don't know what kind of pre-release version numbers PySide & PyQt use, though.

Member

takluyver commented Jan 22, 2013

LooseVersion is not necessarily comparable on Python 3 (whereas StrictVersion is). Something like V('1.0.dev') > V('1.0.3') will throw a TypeError. I don't know what kind of pre-release version numbers PySide & PyQt use, though.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 22, 2013

Member

so I guess we have to write our own version comparison? I hate Python 3's strict comparison rules so much.

Something like:

import re
component_re = re.compile(r'(\d+ | [a-z]+ )', re.VERBOSE)
def V(vstr):
    vlist = component_re.findall(vstr)
    for i in range(len(vlist)):
        try:
            vlist[i] = int(vlist[i])
        except ValueError:
            vlist[i] = float('inf')
    return tuple(vlist)

which would always give a tuple of numbers, interpreting any text ('dev', etc) as infinite (2.1dev will always evaluate as greater than 2.1.x).

Member

minrk commented Jan 22, 2013

so I guess we have to write our own version comparison? I hate Python 3's strict comparison rules so much.

Something like:

import re
component_re = re.compile(r'(\d+ | [a-z]+ )', re.VERBOSE)
def V(vstr):
    vlist = component_re.findall(vstr)
    for i in range(len(vlist)):
        try:
            vlist[i] = int(vlist[i])
        except ValueError:
            vlist[i] = float('inf')
    return tuple(vlist)

which would always give a tuple of numbers, interpreting any text ('dev', etc) as infinite (2.1dev will always evaluate as greater than 2.1.x).

minrk added some commits Jan 25, 2013

add version comparison utility
    
all version strings are comparable by casting string elements to float('inf')
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 25, 2013

Member

I added a NumericalVersion that should always be comparable, regardless of version string and Python version.

Member

minrk commented Jan 25, 2013

I added a NumericalVersion that should always be comparable, regardless of version string and Python version.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 25, 2013

Member

There isn't really a consistent way to compare pre-release versions without knowing about the project's practices. e.g. your implementation implies that 0.13.dev > 0.13.1, but we ourselves use 0.13.dev < 0.13.1 for IPython. That's probably why LooseVersion isn't always comparable - there's no right way to do it.

Looking at their source code, it looks like PySide pre-releases would have a version like 1.1.2~alpha1 - which is comparable, but strictly less than 1.1.2. (ref). PyQt appears to go with snapshot-4.10-f0118624625e (from a development snapshot tarball) - which can't sensibly be compared to anything.

Member

takluyver commented Jan 25, 2013

There isn't really a consistent way to compare pre-release versions without knowing about the project's practices. e.g. your implementation implies that 0.13.dev > 0.13.1, but we ourselves use 0.13.dev < 0.13.1 for IPython. That's probably why LooseVersion isn't always comparable - there's no right way to do it.

Looking at their source code, it looks like PySide pre-releases would have a version like 1.1.2~alpha1 - which is comparable, but strictly less than 1.1.2. (ref). PyQt appears to go with snapshot-4.10-f0118624625e (from a development snapshot tarball) - which can't sensibly be compared to anything.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 25, 2013

Member

I don't actually care that dev versions get compared correctly, as long as they don't raise an error or get interpreted as too old. Anyone running a dev version other than current latest of any given package should not be considered a supported use case. In this way, a snapshot will always pass the minimum version check, and rightly so.

we ourselves use 0.13.dev < 0.13.1 for IPython

We actually use 0.13dev on both sides of 0.13.0. But the case remains that nobody using X.Y.dev of any version other than today's is valid. It seems to me to always be appropriate to interpret dev as 'latest'.

Member

minrk commented Jan 25, 2013

I don't actually care that dev versions get compared correctly, as long as they don't raise an error or get interpreted as too old. Anyone running a dev version other than current latest of any given package should not be considered a supported use case. In this way, a snapshot will always pass the minimum version check, and rightly so.

we ourselves use 0.13.dev < 0.13.1 for IPython

We actually use 0.13dev on both sides of 0.13.0. But the case remains that nobody using X.Y.dev of any version other than today's is valid. It seems to me to always be appropriate to interpret dev as 'latest'.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 25, 2013

Member

Fair enough. I'd be inclined to compare LooseVersion and catch the error, but I think this should work fine as well.

Member

takluyver commented Jan 25, 2013

Fair enough. I'd be inclined to compare LooseVersion and catch the error, but I think this should work fine as well.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2013

Member

If we went with LooseVersion, what should we do if the comparison fails?

Member

minrk commented Jan 27, 2013

If we went with LooseVersion, what should we do if the comparison fails?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 27, 2013

Member

From your description, I think we can just silence the error, so it accepts
any pre-release version that it can't compare.

Member

takluyver commented Jan 27, 2013

From your description, I think we can just silence the error, so it accepts
any pre-release version that it can't compare.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2013

Member

like this?

def check_version(v, check):
    """check version string v ≥ check

    if dev tags result in TypeError for string-number comparison,
    it is assumed that the dependency is satisfied.
    Users on dev branches are responsible for keeping their own packages up to date.
    """
    try:
        return LooseVersion(v) >= LooseVersion(check)
    except TypeError:
        return True
Member

minrk commented Jan 27, 2013

like this?

def check_version(v, check):
    """check version string v ≥ check

    if dev tags result in TypeError for string-number comparison,
    it is assumed that the dependency is satisfied.
    Users on dev branches are responsible for keeping their own packages up to date.
    """
    try:
        return LooseVersion(v) >= LooseVersion(check)
    except TypeError:
        return True
add check_version utility
using LooseVersion and explicitly catching TypeError, rather than the NumericalVersion subclass.
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2013

Member

check_version added instead of NumericalVersion

Member

minrk commented Jan 27, 2013

check_version added instead of NumericalVersion

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 27, 2013

Member

Yep, that looks good to me.

Do we want to leave NumericalVersion and version_tuple around, or just use check_version wherever we want to do this?

Member

takluyver commented Jan 27, 2013

Yep, that looks good to me.

Do we want to leave NumericalVersion and version_tuple around, or just use check_version wherever we want to do this?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2013

Member

I'll remove NumericalVersion

Member

minrk commented Jan 27, 2013

I'll remove NumericalVersion

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2013

Member

unused code removed, should be ready to merge.

Member

minrk commented Jan 27, 2013

unused code removed, should be ready to merge.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 28, 2013

Member

Looks good to me, merging now.

Member

takluyver commented Jan 28, 2013

Looks good to me, merging now.

takluyver added a commit that referenced this pull request Jan 28, 2013

Merge pull request #2831 from minrk/qtversions
avoid string version comparisons in external.qt

@takluyver takluyver merged commit 8f74907 into ipython:master Jan 28, 2013

1 check passed

default The Travis build passed
Details

@minrk minrk referenced this pull request Jan 28, 2013

Merged

future pyzmq compatibility #2852

2 of 2 tasks complete

minrk added a commit that referenced this pull request Mar 10, 2013

Backport PR #2831: avoid string version comparisons in external.qt
We have learned from pyzmq that one should *always* parse version strings
before comparing them.

minrk added a commit that referenced this pull request Mar 12, 2013

Backport PR #2831: avoid string version comparisons in external.qt
We have learned from pyzmq that one should *always* parse version strings
before comparing them.

minrk added a commit that referenced this pull request Mar 20, 2013

Backport PR #2831: avoid string version comparisons in external.qt
We have learned from pyzmq that one should *always* parse version strings
before comparing them.

@takluyver takluyver referenced this pull request May 16, 2013

Closed

Qt version check broken #3327

@minrk minrk deleted the minrk:qtversions branch Mar 31, 2014

yarikoptic added a commit to yarikoptic/ipython that referenced this pull request May 2, 2014

Merge tag 'rel-0.13.2' into debian-01X
release 0.13.2

* tag 'rel-0.13.2': (65 commits)
  release 0.13.2
  add release date
  fix dot syntax error in inheritance diagram
  only upload sdists (backported from master)
  0.13.2 rc2
  update whatsnew
  Backport PR #3118: don't give up on weird os names
  Backport PR #3117: propagate automagic change to shell
  Backport PR #3097: PyQt 4.10: use self._document = self.document()
  note latest backports
  Backport PR #2224: fix css typo
  avoid references to fiel out of directory
  reimport HTML in different section
  add width:100% to vbox for webkit / FF consistency (0.13.2)
  0.13.2.rc1
  generate whatsnew for 0.13.2
  Backport PR #3008: fix cython module so extension for multiarched python
  Backport PR #3013: py3 workaround for reload in cythonmagic
  Backport PR #2831: avoid string version comparisons in external.qt
  Backport PR #2994: expanduser on %%file targets
  ...

yarikoptic added a commit to yarikoptic/ipython that referenced this pull request May 2, 2014

Merge tag 'rel-0.13.2' (mtheirs) into releases
release 0.13.2

* tag 'rel-0.13.2': (65 commits)
  release 0.13.2
  add release date
  fix dot syntax error in inheritance diagram
  only upload sdists (backported from master)
  0.13.2 rc2
  update whatsnew
  Backport PR #3118: don't give up on weird os names
  Backport PR #3117: propagate automagic change to shell
  Backport PR #3097: PyQt 4.10: use self._document = self.document()
  note latest backports
  Backport PR #2224: fix css typo
  avoid references to fiel out of directory
  reimport HTML in different section
  add width:100% to vbox for webkit / FF consistency (0.13.2)
  0.13.2.rc1
  generate whatsnew for 0.13.2
  Backport PR #3008: fix cython module so extension for multiarched python
  Backport PR #3013: py3 workaround for reload in cythonmagic
  Backport PR #2831: avoid string version comparisons in external.qt
  Backport PR #2994: expanduser on %%file targets
  ...

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2831 from minrk/qtversions
avoid string version comparisons in external.qt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment