add EphemeralsBinding.get_range

[davidlatwe]

Problem

In PR #993, new package command ephemerals and intersects were introduced, however it may not work as expect when you wish to disable a package feature (or other term ?) by default as this example.

See demo code below :

from rez.rex_bindings import EphemeralsBinding, intersects
from rez.vendor.version.requirement import Requirement

# Topic: Is foo.bar on ? Default off
# case 1
ephemerals = EphemeralsBinding([Requirement(".foo.bar-1")])
bar_on = intersects(ephemerals.get("foo.bar", "0"), "1")
assert bar_on is True  # It's on.
# case 2
ephemerals = EphemeralsBinding([])
bar_on = intersects(ephemerals.get("foo.bar", "0"), "1")
assert bar_on is True  # Still on !
# case 3 (workaround)
ephemerals = EphemeralsBinding([])
bar_on = intersects(ephemerals.get("foo.bar", "foo.bar-0"), "1")
assert bar_on is False  # Now it's off..

As you can see, intersects still returns True even no ephemeral requirement given, unless you explicitly set default value to foo.bar-0.

Turns out this behavior was because the default value of ephemerals.get got parsed into Requirement obj in intersects :
https://github.com/nerdvegas/rez/blob/6ee535a71b7cf7e04c82db6a786a7cdffe65688f/src/rez/rex_bindings.py#L231-L233

So when no matched ephemeral given like in case 2 above, it was actually intersecting Requirement('0') with VersionRange('1'). Which always intersects !

Feature propose

To overcome this problem and provide a much intuitive interface, I propose to add ephemerals.get_range, which should always return VersionRange object for intersecting.

With this, the above demo code will then be :

# Topic: Is foo.bar on ? Default off
# case 1
ephemerals = EphemeralsBinding([Requirement(".foo.bar-1")])
bar_on = intersects(ephemerals.get_range("foo.bar", "0"), "1")
assert bar_on is True  # It's on.
# case 2
ephemerals = EphemeralsBinding([])
bar_on = intersects(ephemerals.get_range("foo.bar", "0"), "1")
assert bar_on is False  # It's off.

Hope this make sense to you all :)

[nerdvegas]

Hey, good find! I agree this isn't intuitive and needs better syntax. We're on the right track but there are a few things missing.

First, 'intersects' should be updated to raise an exception if it's given a requirement-like object that's a string, but not a valid package request. Eg, intersects("1", "0") should raise rather than return True - "1" is not a valid package requirement, and this will help avoid accidental misuse. Note that intersects should also work for raw ephemeral request strings (eg ".foo-1"), so that should be taken into account (drop leading '.' before checking with PackageRequest).

Second, the wiki docs (Ephemeral-Packages and Package-Commands) need to be updated to reflect this - as you can see I've go the wrong examples there (they probably worked by fluke when I was testing them).

Third - I'm not sure about intersects() being able to take a VersionRange object. It's a little confusing that it can take version/requirement- like objects (and their string equivalents), and range objects, but not string-equivalents of range objects. It may make things clearer to add a "get_request" method to EphemeralBinding instead. Thus:

intersects(ephemerals.get("foo", "0"), "1")  # bad, will raise exception
intersects(ephemerals.get_request("foo", "0"), "1")  # good

Note that the error raised in the first case should say something like "Invalid package request syntax '0'. Did you use request.get/ephemerals.get instead of get_request?"

Forth - as that last point alludes, we need to provide the same function for the request object, since it suffers the same potential misuse.

Fifth - need to ensure that this kind of thing is supported:

intersects(ephemerals.get_request("foo", "==1.2.3"), "1")

[davidlatwe]

Thanks @nerdvegas :)

For the third point :

I'm not sure about intersects() being able to take a VersionRange object. It's a little confusing that it can take version/requirement- like objects (and their string equivalents), and range objects, but not string-equivalents of range objects

Since what intersects() do is intersecting version ranges, isn't that actually make sense to feed it VersionRange object directly ? And, I think it would be better and simpler to restrict it to only accept version range objects and the string-equivalents of them. Further more, I found that "1" can be a valid package name (no error raised when doing PackageRequest("1")). So it may not easy to distinguish between requirement-string-equivalent and range-string-equivalent, nor a best place for implementing that logic.

It may make things clearer to add a "get_request" method to EphemeralBinding instead.

Actually, I think get_range is more intuitive.
From what I understand about ephemeral packages and how I use them, what I need is to query the requested range of one specific ephemeral package. So I suppose the name get_range is better for the scenario. No ?

Disclaimer, above points are based on my usage of ephemerals, so they might be incorrect.
But if they stand... , back to the first point:

Eg, intersects("1", "0") should raise rather than return True - "1" is not a valid package requirement, and this will help avoid accidental misuse.

If we agree letting intersects to only takes version range objects and the string-equivalents, we can ignore this one.

To summarize my points:

• Implement get_range for both EphemeralsBinding and RequirementsBinding
• Change to treat string input as VersionRange's string-equivalent in intersects()

[nerdvegas]

Ouch @ "1" being a valid package name, you're right. In terms of 'intersects' taking range-like objects only, there are two problems there: 1: This feature is already released, and that would break backwards compatibility 2: I wanted the syntax to be as succinct as possible, eg "if intersects(resolve.maya, "2019+"). The docs for intersects state: *"""A boolean function that returns True if the version or version range of the given object, intersects with the given version range"""* I don't think there's any absolutely correct fix for this one. I do agree that get_range is intuitive though, despite it not really matching the types that 'intersects' already accepts. How about: * We go with get_range support in intersects * Wiki is updated to fix incorrect examples, and to explain that get_range() results are accepted. We should also include the _wrong_ way to do it (ie in current examples) as a warning * Ensure that ranges such as "==1" work as expected ? A

…

On Tue, Mar 2, 2021 at 8:06 PM David Lai ***@***.***> wrote: Thanks @nerdvegas <https://github.com/nerdvegas> :) For the third point : I'm not sure about intersects() being able to take a VersionRange object. It's a little confusing that it can take version/requirement- like objects (and their string equivalents), and range objects, but not string-equivalents of range objects Since what intersects() do is intersecting version ranges, isn't that actually make sense to feed it VersionRange object directly ? And, I think it would be better and simpler to restrict it to *only* accept version range objects and the string-equivalents of them. Further more, I found that "1" can be a valid package name (no error raised when doing PackageRequest("1")). So it may not easy to distinguish between requirement-string-equivalent and range-string-equivalent, nor a best place for implementing that logic. It may make things clearer to add a "get_request" method to EphemeralBinding instead. Actually, I think get_range is more intuitive. From what I understand about ephemeral packages and how I use them, what I need is to query the requested range of one specific ephemeral package. So I suppose the name get_range is better for the scenario. No ? Disclaimer, above points are based on my usage of ephemerals, so they might be incorrect. But if they stand... , back to the first point: Eg, intersects("1", "0") should raise rather than return True - "1" is not a valid package requirement, and this will help avoid accidental misuse. If we agree letting intersects to only takes version range objects and the string-equivalents, we can ignore this one. To summarize my points: - Implement get_range for both EphemeralsBinding and RequirementsBinding - Change to treat string input as VersionRange's string-equivalent in intersects() — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1030 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSVZSEPPNSE3Y53YRUDTBSTA5ANCNFSM4YLW6VDA> .

[davidlatwe]

Ouch @ "1" being a valid package name, you're right.

Ouch indeed !

I don't think there's any absolutely correct fix for this one.

Of course :)
I see your concerns now.

• Ensure that ranges such as "==1" work as expected

I have added a few test cases, and this is also validated in there.
And I also adds get_range for RequirementsBinding, this is still needed, right ?

About updating Wiki, I'll try that tomorrow.

[davidlatwe]

I have updated the Wiki pages, but was afraid putting the example code in my very first comment of this PR will make the page too verbose, so I only put a short warning in ephemeral-packages page.

And I also noticed this:

ephemerals = EphemeralsBinding([])
intersects(ephemerals.get("foo"), "1")
# RuntimeError: Invalid type <class 'NoneType'> passed as first arg to 'intersects'

An error raised if not default value given when using ephemerals.get, which I think it should be handled as a valid input, hence the commit c12a8b9.

But wasn't sure if we still need to handle it as VersionRange object, since the intersecting result with VersionRange(None) will always be False. Should we take the short cut to just return False in intersects() ?

[davidlatwe]

Thanks @nerdvegas :D
All good points and fixed !