Add core.version_ranges:resolve_prereleases conf
#13321
Conversation
AbrilRBS
changed the title
core.ranges_resolve_prereleases confcore.ranges_resolve_prereleases conf
There was a problem hiding this comment.
I know it is not as clean, but a global class attribute VersionRange.accept_prereleases would simplify a lot the implementation and avoid having to propagate the parameter everywhere? Most likely better to do it right as you did, but seemed a bit painful to see so many changes to deliver 1 bit of information to the right place.
AbrilRBS
changed the title
core.ranges_resolve_prereleases confcore.version_ranges:resolve_prereleases conf
AbrilRBS
changed the title
core.version_ranges:resolve_prereleases confcore.version_ranges:resolve_prereleases conf
|
Would be great to have an example in the docu telling the story for this feature (Or at the very least tests for CI!) |
conans/model/version_range.py
Outdated
| @@ -96,9 +101,10 @@ def __init__(self, expression): | |||
| def __str__(self): | |||
| return self._expression | |||
|
|
|||
| def __contains__(self, version): | |||
| def contains(self, version, conf_resolve_prerelease=None): | |||
There was a problem hiding this comment.
I'd say this default is not the safest, we might skip some version in range occurrence that needs the conf passed, and we are not passing it. I'd suggest dropping the =None default and having this forced, even if it needs changing more tests.
There was a problem hiding this comment.
This already breaks the version in range syntax, so it would be failing because it wouldn't be able to find the __contains__ operator, but I understand your point, having so many defaults smells a bit :)
There was a problem hiding this comment.
Yeah, I meant that we had some if range.contains(version) without really providing the necessary conf there, which seems mandatory.
conans/client/graph/graph_builder.py
Outdated
| @@ -157,12 +160,12 @@ def _prepare_node(node, profile_host, profile_build, down_options): | |||
| node.conanfile.requires.tool_require(str(tool_require), | |||
| raise_if_duplicated=False) | |||
|
|
|||
| def _initialize_requires(self, node, graph, graph_lock): | |||
| def _initialize_requires(self, node, graph, graph_lock, resolve_prereleases): | |||
There was a problem hiding this comment.
I think we might have another approach here:
if we define here for all requires we define that such a require allows pre-releases, and that is automatically passed to the VersionRange when requested via property, then the number of modifications necessary might be way less?
Just an idea, I am still not super happy with this PR, it is likely that if we remove the =None defaults, it would require even more changes.
| assert not r.contains(Version(v), None) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("version_range, resolve_prereleases, versions_in, versions_out", [ |
There was a problem hiding this comment.
Do you know that multiple pytest.mark.parametrize one on top of the other do the cartesian product? you can reduce these lines 3x with it.
@pytest.mark.parametrize("resolve_prereleases", [True, False, None])
@pytest.mark.parametrize("version_range, versions_in, versions_out", [There was a problem hiding this comment.
Furthermore, you can also factorize the version_range and versions_in/out, and it will be even shorter
conans/test/integration/graph/version_ranges/version_range_conf.py
Outdated
Show resolved
Hide resolved
conans/test/integration/graph/version_ranges/version_range_conf.py
Outdated
Show resolved
Hide resolved
Changelog: Omit
Docs: Omit
Future changelog: Feature: Add
core.version_ranges:resolve_prereleasesconf to control whether version ranges can resolve to prerelease versionsI still have some questions regarding the implementation of the feature itself,
but this is now at a point where it makes sense to open the PR,
even though there are still places that need updating (And so tests are failing)
Specially I'd need to discuss if changing the VersionRange api is ok,
as this removes the
__contains__function in favour of acontainsmethod,to be able to pass it extra arguments, which can't be done with the
inoperator.The logic for the conf is:
True: Always allows to resolve prerelease versions, whatever the expression saysFalse: Never allows prerelease versions to be resolvedNone: Listens to whatever the the range expression saysThe open questions are:
Nonethe best default for an unspecified conf? My guess it that yes, but I better make surecore.version_ranges:resolve_prereleases(Wascore.ranges_resolve_prereleases)