Add include_prerelease and loose option to version range expression

[climblinne]

Require node-semver 0.6.1
Usage example: [~1.2.2,include_prerelease=True,loose=False]

Changelog: (Feature): Add include_prerelease and loose option to version range expression

• Refer to the issue that supports this Pull Request.
• If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
• I've read the Contributing guide.
• I've followed the PEP8 style guides for Python code.
• I've opened another PR in the Conan docs repo to the develop branch, documenting this one. See Add include_prerelease and loose option to version range expression docs#937
• Wait for Commas in version-ranges make input invalid (breaking) #3964
• Add unit tests

Connects to issue #3798

[jgsogo]

I'm sorry, but I need tests for the first part of the function satisfying, the one you are discussing. I would change the function to:

def satisfying(list_versions, versionexpr, output):
    version_range, loose, include_prerelase = _parse_versionexpr(versionexpr)

    candidates = {}
    for v in list_versions:
        ...

    result = max_satisfying(candidates, version_range, loose=loose, include_prerelease=include_prerelease)
    return candidates.get(result)

Then, I need unittests for _parse_versionexpr and max_satisfying. The satisfying function will be covered enough with existing tests.

[climblinne]

@jgsogo: Thanks for your review. I will do the tests later, but before I need some feedback for the syntax as described in the above comments

[danimtb]

Commas are only used for version range >1.1,<2.1 but not for OR conditions >1.1 || 0.8, not for equivalent version like 2.8 and not for semver compatible ~=3.0. So first I would like to clarify if the proposed strict and prerelease options apply to all the configurations above or not.

AFAIK a expression like this would not be possible 🚫 >1.1,<2.0,>1.2 || 0.8 , right?

[jgsogo]

I'm trying things here (https://semver.npmjs.com/) and in fact, commas are never used (we remove them in our code). Anyway, we should keep the possibility of having one comma (it is documented), but remove it for Conan 2.0.

About the parameters, they have these meanings (from the docs):

-l --loose
        Interpret versions and ranges loosely

-p --include-prerelease
        Always include prerelease versions in range matching

With the --include-prerelease you allow the prereleased version to be taken into account (if not, only the ones that appear explicitely in the version_range will be considered). The loose allows some kind of minor-errors on version strings (although the output will always be a correct one).

[climblinne]

Commas are only used for version range >1.1,<2.1 but not for OR conditions >1.1 || 0.8, not for equivalent version like 2.8 and not for semver compatible ~=3.0. So first I would like to clarify if the proposed strict and prerelease options apply to all the configurations above or not.

The commas are just a Conan implementation and removed anyway, before using the semver library.

The strict (loose=False) parameter apply to the version number. So 1.2 would be not a valid version, where 1.2.0 would be fine. The loose variant accept more non standard semver version numbers.

The prerelease option (include_prerelease) gives us the ability, that we can use e.g. development versions (1.2.1-dev.3) instead of old released version (1.2.0). It works on the range side and change the sorting of version numbers.

Both parameter are valid for all configurations. Only ~=3.0 I think is not a valid option, should be ~3.0.

AFAIK a expression like this would not be possible 🚫 >1.1,<2.0,>1.2 || 0.8 , right?

Should be the same >1.2 <2.0 || 0.8 , which would be fine.

[climblinne]

Here are the options to go:

1. Use include_prerelease=True and loose=False (also include_prerelease=False and loose=True are possible, but they are the default values anyway) as parameters separated by a comma from the rest of the range set: [>1.0, <2.0, loose=False, include_prerelease=True]
2. Use strict (loose=False) and prerelease (include_prerelease=True) as keywords. Seperated by comma: [>1.0, <2.0, strict, prerelease]
3. same as 1, separated by a semicolon: [>1.0, <2.0; loose=False; include_prerelease=True]
4. same as 2, separated by a semicolon: [>1.0, <2.0; strict; prerelease]

Which way to go? Anything else?

[jgsogo]

Yes, I think those are all the options. I vote to keep closer to the implementation, so I will go for: [>1.0 || 0.8, loose=True|False, include_prerelease=True|False]. It needs to keep backward compatibility (the Conan comma) and I will let any order in the arguments.

[climblinne]

I also go for number 1.

[jgsogo]

After talking with the team, we agree that the verbose option should be the best/safest way to go. Final syntax examples should include:

libname/<version>@conan/channel
libname/[<version_range_expr>]@conan/channel
libname/[<min>, <max>]@conan/channel   # <-- This should work for backwards compatibility
libname/[<min>, <max>, include_prerelease=False, loose=True]@conan/channel   # <-- This should work for backwards compatibility
libname/[<min>, <max>, loose=True]@conan/channel   # <-- This should work for backwards compatibility
libname/[<version_range_expression>, loose=True, include_prerelease=False]@user/channel
libname/[<version_range_expression>, include_prerelease=False]@user/channel
libname/[<version_range_expression>, loose=False]@user/channel
libname/[<version_range_expression>, include_prerelease=False, loose=True]@user/channel
...

by default, loose=True and include_prerelease=False as it is the default behavior in the library too.

PD.- I'm adding dependency on #3964 as that one should be fixed first (we cannot remove commas)

[climblinne]

@jgsogo Could you give me a hint, how to write test for protected method '_parse_versionexpr'?

[jgsogo]

Yes, of course.

As we are going to separate tests into functional and unittests soon, maybe it is a good time to start. So, create a file conans/tests/unittests/test_client/test_graph/test_range_resolver.py (will need __init__.py for each new folder to become a python module). Then, we can create the unittests:

⚠️ Disclaimer: following code may not be valid Python

import unittest
from conans.client.graph.range_resolver import _parse_versionexpr

class ParseVerionExpr(unittest.TestCase):

    def test_backwards_compatibility(self):
        self.assertEqual(_parse_versionexpr("2.3, 3.2"), ("2.3 3.2", True, False))
        self.assertEqual(_parse_versionexpr("2.3, <=3.2"), ("2.3 <=3.2", True, False))

    def test_only_loose(self):
        self.assertEqual(_parse_versionexpr("2.3 ,3.2, loose=True"), ("2.3 3.2", True, False))
        self.assertEqual(_parse_versionexpr("2.3 3.2, loose=False"), ("2.3 3.2", False, False))
        self.assertEqual(_parse_versionexpr("2.3 3.2, loose  = False"), ("2.3 3.2", False, False))
        self.assertEqual(_parse_versionexpr("2.3 3.2,  loose  = True"), ("2.3 3.2", True, False))
        ...

    def test_only_prerelease(self):
        self.assertEqual(_parse_versionexpr("2.3, 3.2, include_prerelease=False"), ("2.3 3.2", True, False))
        self.assertEqual(_parse_versionexpr("2.3, 3.2, include_prerelease=True"), ("2.3 3.2", True, False))
        ...

    def test_both(self):
        self.assertEqual(_parse_versionexpr("2.3, 3.2, loose=False, include_prerelease=True"), ("2.3 3.2", False, True))
        self.assertEqual(_parse_versionexpr("2.3, 3.2, include_prerelease=True, loose=False"), ("2.3 3.2", False, True))
        ...

    def test_invalid(self):
        self. assertRaisesRegexp(<ExceptionType>, "expected msg", _parse_versionexpr, "2.3, 3.2, unexpected=True")
        self. assertRaisesRegexp(<ExceptionType>, "expected msg", _parse_versionexpr, "2.3, 3.2, loose=Other")
        self. assertRaisesRegexp(<ExceptionType>, "expected msg", _parse_versionexpr, "2.3, 3.2,")
        ...

Our code will be solid like a rock; and this tests, as they are running a very specific function, will run very fast (add as many tests as you want). And more important, as we know that any issue related to the version-range string is correctly handled in this function, we don't have to worry about invalid strings in other tests, each test class should be testing just one functionality. 💪

[climblinne]

There is the following error on the linux test server: OSError: [Errno 28] No space left on device. Could someone help?

[jgsogo]

Thanks!

max_satisfying is a function from semver package, we have to assume that they test it before releasing a new version.

[climblinne]

@jgsogo Are you able to check, what happen to your linux build server (out of memory)?

[jgsogo]

After running again our CI system, it is working (green check 🙌 )

[climblinne]

@jgsogo Could you rerun the last test. This is not an issue from my side.

[climblinne]

@lasote @jgsogo @memsharded Please check your reviews again. Thanks!

Review again, please.

[jgsogo]

LGTM, btw, what's the meaning of a version range like >=1.2.3 <1.(2+1).0?

New changes to review

[memsharded]

Hi @climblinne

Sorry, by mistake, while reviewing, I wanted to do a PR to your branch and instead pushed to it directly, which is something that github allows. The commit is: 3d9b65a

It does:

• it removes an unused output in code and in tests
• applies some formatting: ordering of imports, pep8, line lengths, etc.
• simplified naming in unittests, not necessary to call them "test". Adding missing ``init.py```

Please check and tell me if something wrong, should I revert something? Thanks!

[climblinne]

It does:

• it removes an unused output in code and in tests
• applies some formatting: ordering of imports, pep8, line lengths, etc.
• simplified naming in unittests, not necessary to call them "test". Adding missing ``init.py```

Please check and tell me if something wrong, should I revert something? Thanks!

I put back the 'output' in the code and added again the warning message for #3964. The message was lost on the way. I also added some tests for it.