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.

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

Add `include_prerelease` and `loose` option to version range expression #3898

Merged
merged 16 commits into from Dec 4, 2018

Conversation

@climblinne
Copy link
Contributor

commented Nov 1, 2018

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 conan-io/docs#937
  • Wait for #3964
  • Add unit tests

Connects to issue #3798

Add `include_prerelease` and `loose` option to version range expressi…
…on (New to node-semver 0.5.0)

Usage example: [~1.2.2,include_prerelease=True,loose=False]

Change-Id: I056ac89bf37949701f67abe57455a519d14310ee
Show resolved Hide resolved conans/client/graph/range_resolver.py Outdated
Show resolved Hide resolved conans/client/graph/range_resolver.py Outdated
Show resolved Hide resolved conans/test/model/version_ranges_test.py Outdated
Change keywords to "strict" (loose=False) and "prerelease" (instead o…
…f include_prerelease)

Reactive test for invalid semver expression.

Change-Id: I2f75293f9c0b498eab6ff8d8a2be381f15cf406f

@climblinne climblinne changed the title Add `include_prerelease` and `loose` option to version range expression Add `prerelease` and `strict` option to version range expression Nov 5, 2018

@jgsogo
Copy link
Member

left a comment

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.

Move parsing of version expression into separate function
Change-Id: Ib5209ef611295ce71c4b94f2b93e09a99c56c636
@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

I also go for number 1.

@jgsogo jgsogo self-assigned this Nov 21, 2018

@jgsogo

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

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)

Change back to "include_prerelease=True" and "loose=False" option.
Change-Id: I388e9eba183e640fcf462546bb502e137997111f

@climblinne climblinne changed the title Add `prerelease` and `strict` option to version range expression Add `include_prerelease` and `loose` option to version range expression Nov 26, 2018

climblinne added some commits Nov 26, 2018

Use python 2.7 compatible regex syntax
Change-Id: I21765c9086ca192f90e5ed7e46a608e8d765a922
Update 'node-semver' version to 0.6.1
Change-Id: I182bb1190f78d9648d30a5fe5dbc2a0a441334f6
@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

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

@jgsogo

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

Add deprecated message for comma usage in version range
Change-Id: Ifc2872ad1c4f9d40328e360777015727fdf20357
@jgsogo

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Thanks!

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

@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

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

@jgsogo

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

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

climblinne and others added some commits Nov 29, 2018

Check version range in 'satisfying' function
Added some tests for invalid version ranges
Updated warning message, when using commas in version range.

Change-Id: Ie76dfa0769f434e710fc21ddcaab48841ec5b54e
Updated version range regex and corrected test to fail correctly with…
… ConanException

Change-Id: If4f3123ef2ae3061b6556d89417d0cd4c898e197
Reverted some tests, because "" is a valid range.
Add asterix and caret symbol to version regex

Change-Id: I3fa9cae1a6b8801e252e9bca2efe860a0ec38d68
Add round brackets to version range regex
Change-Id: I9024e63a8387a388cd8f486b05051fc5197228c8
@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

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

@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

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

@lasote lasote added this to the 1.10 milestone Dec 3, 2018

Review again, please.

@jgsogo
Copy link
Member

left a comment

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

New changes to review

@ghost ghost assigned memsharded Dec 3, 2018

@ghost ghost added stage: review and removed stage: in-progress labels Dec 3, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

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!

Show resolved Hide resolved conans/client/graph/range_resolver.py Outdated
Show resolved Hide resolved conans/client/graph/range_resolver.py Outdated
Show resolved Hide resolved conans/client/graph/range_resolver.py
Add deprecated message for comma usage in version range.
Change assertion to raise ConanException in '_parse_versionexpr'.
Used named groups in regex matching.

Change-Id: Ife07c32062eb192997b5ca1700392111519b93b9
@climblinne

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

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.

@lasote lasote merged commit 13c110c into conan-io:develop Dec 4, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Dec 4, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Add `include_prerelease` and `loose` option to version range expressi…
…on (conan-io#3898)

* Add `include_prerelease` and `loose` option to version range expression (New to node-semver 0.5.0)
Usage example: [~1.2.2,include_prerelease=True,loose=False]

Change-Id: I056ac89bf37949701f67abe57455a519d14310ee

* Change keywords to "strict" (loose=False) and "prerelease" (instead of include_prerelease)
Reactive test for invalid semver expression.

Change-Id: I2f75293f9c0b498eab6ff8d8a2be381f15cf406f

* Move parsing of version expression into separate function

Change-Id: Ib5209ef611295ce71c4b94f2b93e09a99c56c636

* Change back to "include_prerelease=True" and "loose=False" option.

Change-Id: I388e9eba183e640fcf462546bb502e137997111f

* Use python 2.7 compatible regex syntax

Change-Id: I21765c9086ca192f90e5ed7e46a608e8d765a922

* Update 'node-semver' version to 0.6.1

Change-Id: I182bb1190f78d9648d30a5fe5dbc2a0a441334f6

* Add deprecated message for comma usage in version range

Change-Id: Ifc2872ad1c4f9d40328e360777015727fdf20357

* Add unit tests for '_parse_versionexpr' function

Change-Id: I85804296068d2252553e1c84ba4b753b957cfb34

* Check version range in 'satisfying' function

Added some tests for invalid version ranges
Updated warning message, when using commas in version range.

Change-Id: Ie76dfa0769f434e710fc21ddcaab48841ec5b54e

* work on version ranges

* Updated version range regex and corrected test to fail correctly with ConanException

Change-Id: If4f3123ef2ae3061b6556d89417d0cd4c898e197

* Reverted some tests, because "" is a valid range.
Add asterix and caret symbol to version regex

Change-Id: I3fa9cae1a6b8801e252e9bca2efe860a0ec38d68

* Add round brackets to version range regex

Change-Id: I9024e63a8387a388cd8f486b05051fc5197228c8

* minor improvements

* missing init.py

* Add deprecated message for comma usage in version range.
Change assertion to raise ConanException in '_parse_versionexpr'.
Used named groups in regex matching.

Change-Id: Ife07c32062eb192997b5ca1700392111519b93b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.