Add invertVersionRange and invertVersionIntervals to Distribution.Version

[ddssff]

Reference #925

[23Skidoo]

Looks OK, but can you please make all lines fit into 80 columns?

[dcoutts]

This module is one of the few modules in Cabal that does actually have proper QuickCheck tests. Would you mind adding properties for the new functions that specify what they do. It should be straightforward given the interpretation as not within a version range. It's quite easy to get these bits of code wrong, I certainly did with the originals in the module (and was surprised that one property I expected to hold does not), so I think it's worth doing.

[ddssff]

I looked for these but they seem to have been lost in the transition to git. For example, the darcs repo had a directory Cabal/tests/Test/Distribution but the git repo does not.

[dcoutts]

@ddssff oh you're right.

Let's blame @tibbe :-)

commit 1e2ea93c3d509347fd990bdb3f947e72bc2b69de
Author: Johan Tibell <johan.tibell@gmail.com>
Date:   Mon Aug 6 11:31:18 2012 -0700

    Delete more dead test code

 Cabal/tests/Test/Distribution/Version.hs
...

@ddssff Would you be kind enough to resurrect this test module?

[ddssff]

I'm surprised that travis build passed - the parsing and pretty printing tests fail here. And when I click on Details it says the build finished a month ago...

[ddssff]

Ugh, I need to change the signature of foldVersionRange and friends.

[dcoutts]

@ddssff wait, why did you need to add InvertVersionRange as a full-on data constructor for VersionRange? I didn't notice you adding explicit syntax for it, I thought it was purely an operation. If you keep it as before in your first patch-set then you don't need to change the fold function.

[ddssff]

I would be happy to not create a constructor and not change the fold function. I wasn't sure whether it was necessary.

[dcoutts]

@ddssff Ok. Yes, I think adding a constructor and changing the fold are not necessary. It can be purely an operation and not syntax.

[ddssff]

Two of the quickcheck properties are failing for the ghc-7.4 build only, but they are not related to invert. Should I disable them, disable the 7.4 travis run, ifdef them, or leave them be? Or make the 7.4 run an allowed failure.

[dcoutts]

Oh? Which ones?

I guess we could disable them for now, but we're likely to just forget if we do.

Great work in resurrecting the tests! Much appreciated.

[ddssff]

Glad to help with the tests - these are the failing results:

  Range Property 29: [Failed]
*** Failed! Falsifiable (after 41 tests): 
EarlierVersion (Version {versionBranch = [1,0], versionTags = []})
UnionVersionRanges (ThisVersion (Version {versionBranch = [1], versionTags = []})) (EarlierVersion (Version {versionBranch = [1], versionTags = []}))
(used seed -3395209717927147713)

  Range Property 31: [Failed]
*** Failed! Falsifiable (after 30 tests): 
AnyVersion
UnionVersionRanges (ThisVersion (Version {versionBranch = [0], versionTags = []} (VersionRangeParens (UnionVersionRanges (ThisVersion (Version {versionBranch = [0,0], versionTags = []})) (LaterVersion (Version {versionBranch = [0,0], versionTags = []}))))

Which correspond to these properties:

--FIXME: see equivalentVersionRange for details
prop_simplifyVersionRange2'' :: VersionRange -> VersionRange -> Property
prop_simplifyVersionRange2'' r r' =
  r /= r' && r `equivalentVersionRange` r' ==>
       simplifyVersionRange r == simplifyVersionRange r'
    || isNoVersion r
    || isNoVersion r'

prop_to_intervals_canonical :: VersionRange -> VersionRange -> Property
prop_to_intervals_canonical r r' =
  r /= r' && r `equivalentVersionRange` r' ==>
    toVersionIntervals r == toVersionIntervals r'

Is it worth worrying about tests that only fail under 7.4?