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 type annotations #1761

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented May 19, 2024

This is a first pass at adding type annotations throughout the code-base. Mypy is not fully passing yet, but it's getting close.

Addresses #1631

@chadrik chadrik requested a review from a team as a code owner May 19, 2024 17:38
Copy link

linux-foundation-easycla bot commented May 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -327,7 +327,7 @@ class Rule(object):
"""Base package filter rule"""

#: Rule name
name = None
name: str
Copy link
Contributor Author

@chadrik chadrik May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern appears on abstract base classes in a number of places, and we have 3 choices:

Option 1:

name: str | None = None

typically produces many spurious mypy errors, because the code assumes the value is a string everywhere, and the None-case is not handled.

Option 2:

name: str

only safe if we're sure that the name attribute is always accessed on concrete sub-classes, otherwise will result in an AttributeError since this does not actually create an attribute.

Option 3:

@property
@abstractmethod
def name(self) -> str:
    raise NotImplementedError

Provides us some guarantee that sub-classes actually implement name, but it cannot be accessed from the class, only from an instance.

def __eq__(self, other):
def __eq__(self, other: object) -> bool:
if not isinstance(other, FallbackComparable):
return NotImplemented
Copy link
Contributor Author

@chadrik chadrik May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy strongly prefers that comparison functions define the type of other as object. by returning NotImplemented python will then defer to the other object in the comparison to handle equality if it implements it. This should not change the behavior as long as other is always FallbackComparable within Rez.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation comments should go in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do.

pass

def iter_packages(self) -> Iterator[Package]:
raise NotImplementedError
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All subclasses of PackageFamilyResource implement this method, so it appears to be considered an abstractmethod of PackageFamilyResource. Defining it simplifies some type annotation problems.

@property
@abstractmethod
def parent(self) -> PackageRepositoryResource:
raise NotImplementedError
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same situation here as with PackageFamilyResource.iter_packages.

@property
@abstractmethod
def parent(self) -> PackageRepositoryResource:
raise NotImplementedError
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here as well.

@@ -319,6 +326,7 @@ def get_variant(self, index=None):
for variant in self.iter_variants():
if variant.index == index:
return variant
return None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy prefer explicit return None statements

raise ResolvedContextError(
"Cannot perform operation in a failed context")
return _check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy does not like these decorators defined at the class-level.

@chadrik
Copy link
Contributor Author

chadrik commented May 19, 2024

Protocol and TypedDict are not available in the typing module until python 3.8.

We have a few options:

  1. vendor typing_extensions
  2. remove use of these typing classes until Rez drops support for python 3.7
  3. I can create a mock of Protocol and trick mypy into using it which is safe because it has no runtime behavior. Doing the same thing for TypedDict is more complicated, but possible.

Copy link

codecov bot commented May 19, 2024

Codecov Report

Attention: Patch coverage is 83.19328% with 180 lines in your changes missing coverage. Please review.

Project coverage is 59.16%. Comparing base (491497f) to head (dd86192).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/rez/suite.py 53.33% 20 Missing and 1 partial ⚠️
src/rez/version/_version.py 88.81% 11 Missing and 7 partials ⚠️
src/rez/package_order.py 73.43% 13 Missing and 4 partials ⚠️
src/rez/resolved_context.py 75.92% 11 Missing and 2 partials ⚠️
src/rez/solver.py 94.03% 10 Missing and 3 partials ⚠️
src/rez/package_resources.py 77.55% 9 Missing and 2 partials ⚠️
src/rez/build_system.py 65.51% 9 Missing and 1 partial ⚠️
src/rez/packages.py 82.75% 9 Missing and 1 partial ⚠️
src/rez/build_process.py 75.00% 5 Missing and 2 partials ⚠️
src/rez/utils/data_utils.py 79.31% 4 Missing and 2 partials ⚠️
... and 23 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1761      +/-   ##
==========================================
- Coverage   59.30%   59.16%   -0.15%     
==========================================
  Files         126      127       +1     
  Lines       17210    17452     +242     
  Branches     3015     3049      +34     
==========================================
+ Hits        10206    10325     +119     
- Misses       6319     6408      +89     
- Partials      685      719      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chadrik
Copy link
Contributor Author

chadrik commented May 20, 2024

I got bored and added lots more, particularly focused on the solver module. Once the solver module is complete, we can experiment with compiling it to a c-extension using mypyc, which could provide a big speed boost!

@chadrik
Copy link
Contributor Author

chadrik commented May 21, 2024

I now have rez.solver compiling as a C-extension with all tests passing. I'm very interested to see how the performance compares. Does anyone want to volunteer to help put together a performance comparison? Are there any known complex collection of packages to test against?

self.dirty = True
return super().append(*args, **kwargs)
if not TYPE_CHECKING:
def append(self, *args, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this class inherits from list it's easier to rely on the type hints coming from that base class than to redefine them here, so we hide them by placing them behind not TYPE_CHECKING. In reality, the runtime value of TYPE_CHECKING is always False.

def get_plugin_class(self, plugin_type: str, plugin_name: str, expected_type: type[T]) -> type[T]:
pass

def get_plugin_class(self, plugin_type, plugin_name, expected_type=None):
"""Return the class registered under the given plugin name."""
plugin = self._get_plugin_type(plugin_type)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new argument here to validate the returned result. This provides both runtime and static validation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could plugin_type be an enum or something like that? This would remove the need for expected_type right? Or maybe we could use overloads with Literals for expected_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this uses type vars to describe a generic function, i.e. a function where the types of its arguments are related to each other. In our case, the type of the argument expected_type is related to the output type.

Using a literal would mean we would have to define a literal value and a function overload for every possible output type, like this:

    @overload
    def get_plugin_class(self, plugin_type: str, plugin_name: str, expected_type: Literal["Foo"]) -> type[Foo]:
        pass

    @overload
    def get_plugin_class(self, plugin_type: str, plugin_name: str, expected_type: Literal["Bar"]) -> type[Bar]:
        pass

In a plugin environment where users can define their own types, we obviously cannot define a string constant for every possible type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users can't define their own types. They can only create new plugin of some pre-defined types. Anybody adding new plugin types should do so in rez itself and not outside.

"""Perform a package resolve, and store the result.

Args:
package_requests (list[typing.Union[str, PackageRequest]]): request
package_requests (list[typing.Union[str, Requirement]]): request
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that everywhere that we've documented types as PackageRequest, they appear to actually be Requirement. I'm not sure if there any real-world exceptions to this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's really supposed to be a PackageRequest if I'm not mistaken. But there is technically no differences between the two once the instantiated since PackageRequest inherits from Requirement and only overloads __init__ to check the inputs.

Copy link
Contributor Author

@chadrik chadrik Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kind of haphazard use and documentation of PackageRequest and PackageRequest results in some very difficult situations to accurately add type annotations. If you want to see for yourself, check out the code, change this to PackageRequest and observe the new errors produced by mypy.

@@ -884,7 +912,7 @@ def _rt(t):
return

_pr("resolved packages:", heading)
rows = []
rows3: list[tuple[str, str, str]] = []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't redefine types with mypy, so you need to use new variable names.

@@ -88,7 +88,7 @@ def shell(self):
args = ['ps', '-o', 'args=', '-p', str(parent_pid)]
proc = sp.Popen(args, stdout=sp.PIPE)
output = proc.communicate()[0]
shell = os.path.basename(output.strip().split()[0]).replace('-', '')
shell = os.path.basename(output.decode().strip().split()[0]).replace('-', '')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output is bytes in python3, so need to call decode()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use text=True, so it should be string isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't use text=True here. I could pass text=True to Popen.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if TYPE_CHECKING:
cached_property = property
else:
class cached_property(object):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's much easier to pretend that cached_property is property than to type hint all the subtleties of a descriptor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we loose the uncache method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typing.TYPE_CHECKING always resolve to False at runtime and True only during static analysis. So the code within the if TYPE_CHECKING block will never run. It's a way to simplify certain type analysis situations that arise.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typing.TYPE_CHECKING always resolve to False

I know, but we are loosing stuff during typing. That's my whole point (the same apply to all my comments that are similar to this one).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what way would the static analysis be degraded by using property instead of cached_property? To my knowledge, they are functionally equivalent from a static analysis POV: the types returned are the same.

self.depth_counts: dict
self.solve_begun: bool
self.solve_time: float
self.load_time: float
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these values are never actually None, so we define the types here, and their values are assigned in _init()

@@ -74,9 +97,6 @@ class SolverStatus(Enum):
cyclic = ("The solve contains a cycle.", )
unsolved = ("The solve has started, but is not yet solved.", )

def __init__(self, description):
self.description = description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypyc did not like adding a new __init__ for Enum, so it was simple enough to replace this attribute with a call to the enum value.

"""Reset the solver, removing any current solve."""
if not self.request_list.conflict:
phase = _ResolvePhase(self.request_list.requirements, solver=self)
phase = _ResolvePhase(solver=self)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a bug: _ResolvePhase only takes one argument. mypy to the rescue.

@chadrik
Copy link
Contributor Author

chadrik commented May 21, 2024

I found rez-benchmark. Interestingly, rez is slower with the compiled rez.solver. It could be because there are many modules and classes used by rez.solver which have not been compiled.

I probably won't have time to dig into this much more, but once this PR is merged I'll make a new PR with the changes necessary for people to test the compiled version of rez.

@chadrik
Copy link
Contributor Author

chadrik commented May 21, 2024

Note: this PR likely invalidates #1745

@chadrik
Copy link
Contributor Author

chadrik commented Jun 5, 2024

@instinct-vfx Can you or someone from the Rez group have a look at this, please?

@JeanChristopheMorinPerso JeanChristopheMorinPerso added the Blocked by CLA Waiting on CLA to be signed label Jun 22, 2024
return self.build_system.working_dir

def build(self, install_path=None, clean=False, install=False, variants=None):
def build(self, install_path: str | None = None, clean: bool = False,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using str | None means we need to drop support for python 3.7. I'm not sure we are ready for this yet.

Copy link
Contributor Author

@chadrik chadrik Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, use of str | None is safe in python 3.7 as long as you use from __future__ import annotations. This backports behavior from python 3.9 that ensures that type annotations are recorded as strings within __annotations__ attributes, which means they are not evaluated at runtime unless inspect.get_annoations is called. The effect of from __future__ import annotations is that you can put basically anything you want into an annotation, it doesn't need to be valid at runtime.

The only thing breaking python 3.7 compatibility here is the use of TypedDict and Protocol, as mentioned in another comment. I presented 3 options for preserving the use of these classes in the other comment.

I noticed that the only python 3.7 tests that are currently run are for MacOS, which I took as an indicator that python 3.7 would be dropped soon. Is there a schedule for deprecation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I fixed the python 3.7 compatibility issue with TypedDict and Protocol, so that should not be a blocker anymore.

@JeanChristopheMorinPerso
Copy link
Member

@chadrik You need to sign the CLA before we can even start to look at the PR.

@chadrik
Copy link
Contributor Author

chadrik commented Jun 22, 2024

@JeanChristopheMorinPerso

@chadrik You need to sign the CLA before we can even start to look at the PR.

I work for Scanline, which is owned by Netflix, and I'm meeting with our CLA manager on Monday. I made this contribution on my own time: can choose individual vs corporate CLA on a per PR basis?

@JeanChristopheMorinPerso
Copy link
Member

I made this contribution on my own time: can choose individual vs corporate CLA on a per PR basis?

I don't think you "can" but your account can be associated to an an ICLA and a CCLA. But I'm not a lawyer so I can't help more than that. If you and or your employer/CLA manager have questions, you can contact the LF support by following the link in the EasyCLA comment: #1761 (comment).

@chadrik chadrik force-pushed the typing branch 4 times, most recently from e73c6c1 to 961420b Compare June 22, 2024 23:10
@chadrik
Copy link
Contributor Author

chadrik commented Jul 1, 2024

CLA is signed!

@@ -143,12 +167,12 @@ def __init__(self, working_dir, opts=None, package=None,
self.opts = opts

@classmethod
def is_valid_root(cls, path):
def is_valid_root(cls, path: str, package=None) -> bool:
"""Return True if this build system can build the source in path."""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sub-classes are expected to have the package argument. I think this was just an oversight on the base class.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What type should it have?

@@ -614,13 +633,13 @@ def from_pod(cls, data):
)


class PackageOrderList(list):
class PackageOrderList(List[PackageOrder]):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we use typing.List here instead of list because list does not become indexable at runtime until python 3.9. It's still safe to use list[X] inside annotations as long as we use from __future__ import annotations.

@JeanChristopheMorinPerso JeanChristopheMorinPerso removed the Blocked by CLA Waiting on CLA to be signed label Jul 1, 2024
@chadrik
Copy link
Contributor Author

chadrik commented Jul 18, 2024

Any thoughts on this PR?

@JeanChristopheMorinPerso
Copy link
Member

Hey @chadrik, I made a first good read last week and I'll try to do another one soon. If you have the time, I would really love to see a GitHub Actions workflow that would run mypy on all pull requests.

@chadrik
Copy link
Contributor Author

chadrik commented Jul 21, 2024

I would really love to see a GitHub Actions workflow that would run mypy on all pull requests.

Me too!

The challenge is that there are still a lot of errors. These are not the result of incorrect annotations, but rather due to code patterns which are difficult to annotate correctly without more invasion refactors. For example, there are quite a few objects which dynamically generate attributes, and that's a static typing anti-pattern.

If you'd like an Action that runs mypy but allows failure for now, that's pretty easy, but if you want failures to block PRs, that'll take a lot more work. I'd prefer not to make that a blocker to merging this, though, because I've had to rebase and fix merge conflicts a few times already.

I do have a plan for how we can get to zero failures in the mid-term: I wrote a tool which allows you to specify patterns for errors to ignore, but I need to update it.

@JeanChristopheMorinPerso
Copy link
Member

I think we can introduce a workflow that will fail for newly introduced errors and warnings. I'm sure someone already thought of that somewhere and we can probably re-use what they did?

Basically, I'd like to verify that your changes work as expected and that we don't regress in the future and that new code is typed hint. Mypy can also be configured on a per module basis right?

@chadrik
Copy link
Contributor Author

chadrik commented Aug 5, 2024

@JeanChristopheMorinPerso I used a project called mypy-baseline to create a set of ignore patterns that can be filtered out until they're resolved: https://mypy-baseline.orsinium.dev/usage

If new errors are introduced they will need to be resolved (i.e. errors not filtered by the established baseline regexes). The regexes are bound to particular files but not to line numbers. The mypy CI job will pass if a developer fixes an error, but in this case it is good practice to set a new baseline using mypy | mypy-baseline sync --ignore-categories=note. When creating a baseline, care must be taken to use the same version of python and mypy.

@dbr
Copy link
Contributor

dbr commented Aug 9, 2024

Not sure if it was discussed already, but is it feasible to only use # typing: ignore comments to suppress existing known errors?

Main benefit would be people using mypy integration in their editors wouldn't be flooded with the errors ignored by mypy-baseline

I think mypy also warns when the ignore is no longer needed, so if some refactoring resolves the issue, the comments are easy to clean up

@chadrik
Copy link
Contributor Author

chadrik commented Aug 11, 2024

There are about 300 errors, so using inline ignores has the downside of littering the code with type comments. It also means that developers need to be careful to move the ignore comments to the proper locations when refactoring code, and the solution not always obvious. I think at this stage what I’ve presented here is a very low effort way to ease this project into type annotations. With the mypy-baseline project there is a simple way to just bulk add problems to the ignore list, if necessary.

FWIW, most of the remaining errors will require some pretty major refactors to resolve — meaning we probably need to replace the schema objects with something like dataclasses or pydantic to remove dynamic attribute generation. If folks are interested in taking on some more substantial changes to get the code base fully covered, I’m happy to do that.

@chadrik
Copy link
Contributor Author

chadrik commented Aug 23, 2024

Shall we get this merged?

@chadrik
Copy link
Contributor Author

chadrik commented Sep 11, 2024

Hi! Is there anything I can do to get this across the line?

Signed-off-by: Chad Dombrova <[email protected]>
@JeanChristopheMorinPerso
Copy link
Member

Rebased to get all the latest changes from main.

Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second batch of comments. Note that I'm not yet done with the review. Please don't push changes until I'm done.

@@ -67,9 +89,10 @@ def get_valid_build_systems(working_dir, package=None):
return clss


def create_build_system(working_dir, buildsys_type=None, package=None, opts=None,
def create_build_system(working_dir: str, buildsys_type: str | None = None,
package=None, opts=None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
package=None, opts=None,
package: Package | None = None, opts: argparse.Namespace | None = None,

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other comment, I'd prefer for new annotations to be done in a new PR, so that we can keep the scope of this PR under control. There are literally thousands of unannotated arguments that still need to be dealt with.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see, the current typing seems arbitrary. Some stuff was typed, some others don't and I'm not too sure what the logic is. I think it would be preferable to change the whole signature instead of just changing half of it since we are anyway changing it.

"""Return the name of the build system, eg 'make'."""
raise NotImplementedError

def __init__(self, working_dir, opts=None, package=None,
write_build_scripts=False, verbose=False, build_args=[],
def __init__(self, working_dir: str, opts=None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __init__(self, working_dir: str, opts=None,
def __init__(self, working_dir: str, opts: argparse.Namespace | None = None,

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for new annotations to be done in a new PR, so that we can keep the scope of this PR under control. There are literally thousands of unannotated arguments that still need to be dealt with.

Comment on lines +136 to 137
write_build_scripts: bool = False, verbose: bool = False, build_args=[],
child_build_args=[]):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
write_build_scripts: bool = False, verbose: bool = False, build_args=[],
child_build_args=[]):
write_build_scripts: bool = False, verbose: bool = False, build_args" Sequence[str]=[],
child_build_args: Sequence[str]=[]):

? (we could potentially do this later since we'd need to remove the default lists.

@@ -143,12 +167,12 @@ def __init__(self, working_dir, opts=None, package=None,
self.opts = opts

@classmethod
def is_valid_root(cls, path):
def is_valid_root(cls, path: str, package=None) -> bool:
"""Return True if this build system can build the source in path."""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What type should it have?

def __eq__(self, other):
def __eq__(self, other: object) -> bool:
if not isinstance(other, FallbackComparable):
return NotImplemented

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation comments should go in the code.

@@ -42,8 +42,8 @@ def get_patched_request(requires, patchlist):
'^': (True, True, True)
}

requires = [Requirement(x) if not isinstance(x, Requirement) else x
for x in requires]
requires: list[Requirement | None] = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it really contain a None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code below says:

        for i, req in enumerate(requires):
            if req is None or req.name != name:
                continue

One way to try to find out would be to annotate the function and see if mypy detects any cases where None is passed:

def get_patched_request(requires: list[Requirement | str], patchlist):

@@ -555,7 +557,8 @@ def _difftool(self):


# singleton
platform_ = None
# FIXME: is is valid for platform_ to be None?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes, say on solaris, freebsd, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowing it to be None causes tons of mypy errors because there are lots of places where platform_ is used without checking if it's None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to be perfectly clear, these errors indicate that there are many places with the rez code that will fail at runtime if platform_ is None.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was just answering the question. It will fail if it's None. We could fix this by simply adding an else clause and raise a exception with a clear error message.

if TYPE_CHECKING:
# this is not available in typing until 3.11, but due to __future__.annotations
# we can use it without really importing it
from typing import Self

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it impossible to run mypy with python < 3.11 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, because this is inside a if TYPE_CHECKING block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I explained in my other comment code within an if TYPE_CHECKING does not actually run.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note how I said "run mypy". Like, if we were to run mypy with python_version = 3.7 for example.

if (self.name_ != other.name_) or (self.range is None):
return False
if self.conflict:
return (other.version_ in self.range_)
else:
return (other.version_ not in self.range_)
else:
return NotImplemented

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this raise an NotImplementedError instead?

Copy link
Contributor Author

@chadrik chadrik Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. NotImplemented is a singleton that is used to indicate "I don't support this comparison", and python will fall back to other object's special method to see if it supports the comparison.

https://docs.python.org/3/reference/datamodel.html#object.__ge__

When no appropriate method returns any value other than NotImplemented, the == and != operators will fall back to is and is not, respectively.

This is a fundamental part of how comparison works in python, and returning NotImplemented is the most correct behavior.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should we allow comparing with anything else than a Requirement or VersionedObject? I wouldn't consider this function a comparator per say.

if self._str is None:
pre_str = '~' if self.negate_ else ('!' if self.conflict_ else '')
range_str = ''
sep_str = ''

range_ = self.range_
if self.negate_:
# Note: the only time that range_ is None is if self.negate_ is True
if self.negate_ or range_ is None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right thing to do. Why should range_ be None?

Copy link
Contributor Author

@chadrik chadrik Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the init code:

        m = self.sep_regex.search(s)
        if m:
            i = m.start()
            self.name_ = s[:i]
            req_str = s[i:]
            if req_str[0] in ('-', '@', '#'):
                self.sep_ = req_str[0]
                req_str = req_str[1:]
            self.range_ = VersionRange(
                req_str, invalid_bound_error=invalid_bound_error)
            if self.negate_:
                self.range_ = ~self.range_
        elif self.negate_:
            self.name_ = s
            # rare case - '~foo' equates to no effect
            self.range_ = None
        else:
            self.name_ = s
            self.range_ = VersionRange()

Notice that if self.negate_ is True, self._range_ is set to None.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants