style: update type hints and enable type checking in ci for apt.py #151
Conversation
james-garner-canonical
force-pushed
the
2025-01/types/enable-type-checking-for-apt
branch
from
69379d7 to
2453b0f
Compare
| matches = epoch_matcher.search(version).groupdict() | ||
| return matches.get("epoch", ""), matches.get("version") | ||
| result = epoch_matcher.search(version) | ||
| assert result is not None | ||
| matches = result.groupdict() | ||
| return matches.get("epoch", ""), matches["version"] |
There was a problem hiding this comment.
This issue here is that re.Pattern.search(...) can return None. Presumably it never does on well formed version strings, so we can just assert.
Note also the change to returning matches["version"] instead of matches.get("version"), as the latter is also typed as possibly returning None, which contradicts the function signature. The regex match should always have an entry for "version" since it matches .*.
| try: | ||
| matches = dpkg_matcher.search(line).groupdict() | ||
| package_status = matches["package_status"] | ||
|
|
||
| if not package_status.endswith("i"): | ||
| logger.debug( | ||
| "package '%s' in dpkg output but not installed, status: '%s'", | ||
| package, | ||
| package_status, | ||
| ) | ||
| break | ||
|
|
||
| epoch, split_version = DebianPackage._get_epoch_from_version(matches["version"]) | ||
| pkg = DebianPackage( | ||
| matches["package_name"], | ||
| split_version, | ||
| epoch, | ||
| matches["arch"], | ||
| PackageState.Present, | ||
| ) | ||
| if (pkg.arch == "all" or pkg.arch == arch) and ( | ||
| version == "" or str(pkg.version) == version | ||
| ): | ||
| return pkg | ||
| except AttributeError: | ||
| result = dpkg_matcher.search(line) | ||
| if result is None: | ||
| logger.warning("dpkg matcher could not parse line: %s", line) | ||
| continue | ||
| matches = result.groupdict() | ||
| package_status = matches["package_status"] | ||
|
|
||
| if not package_status.endswith("i"): | ||
| logger.debug( | ||
| "package '%s' in dpkg output but not installed, status: '%s'", | ||
| package, | ||
| package_status, | ||
| ) | ||
| break | ||
|
|
||
| epoch, split_version = DebianPackage._get_epoch_from_version(matches["version"]) | ||
| pkg = DebianPackage( | ||
| name=matches["package_name"], | ||
| version=split_version, | ||
| epoch=epoch, | ||
| arch=matches["arch"], | ||
| state=PackageState.Present, | ||
| ) | ||
| if (pkg.arch == "all" or pkg.arch == arch) and ( | ||
| version == "" or str(pkg.version) == version | ||
| ): | ||
| return pkg |
There was a problem hiding this comment.
Originally the whole loop body is wrapped in a try ... except AttributeError, again due to re.Pattern.search(...) potentially returning None instead of a re.Match. This change explicitly checks if the search result is None, allowing the loop body to be dedented.
| vals["Package"], | ||
| split_version, | ||
| epoch, | ||
| vals["Architecture"], | ||
| PackageState.Available, | ||
| name=vals["Package"], | ||
| version=split_version, | ||
| epoch=epoch, | ||
| arch=vals["Architecture"], | ||
| state=PackageState.Available, |
There was a problem hiding this comment.
Just using keyword arguments because why not
| def _listify(self, revision: str) -> list[str]: | ||
| """Split a revision string into a listself. | ||
| def _listify(self, revision: str) -> list[str | int]: | ||
| """Split a revision string into a list. |
There was a problem hiding this comment.
Return type is a list that alternates between strings and ints. This will come up later
| @@ -651,31 +657,38 @@ def _compare_revision_strings(self, first: str, second: str): # noqa: C901 | |||
| # explicitly raise IndexError if we've fallen off the edge of list2 | |||
| if i >= len(second_list): | |||
| raise IndexError | |||
| other = second_list[i] | |||
There was a problem hiding this comment.
Use a local variable to allow type narrowing -- and it's more efficient anyway
| assert isinstance(other, int) | ||
| if item > other: | ||
| return 1 | ||
| if item < second_list[i]: | ||
| if item < other: | ||
| return -1 | ||
| else: | ||
| # string comparison | ||
| return self._dstringcmp(item, second_list[i]) | ||
| assert isinstance(other, str) | ||
| return self._dstringcmp(item, other) |
There was a problem hiding this comment.
This is that list of alternating integers and strings -- well a pair of them being compared
| except IndexError: | ||
| # rev1 is longer than rev2 but otherwise equal, hence greater | ||
| # ...except for goddamn tildes | ||
| if first_list[len(second_list)][0][0] == "~": | ||
| # FIXME: bug?? we return 1 in both cases | ||
| # FIXME: first_list[len(second_list)] should be a string, why are we indexing to 0 twice? | ||
| if first_list[len(second_list)][0][0] == "~": # type: ignore | ||
| return 1 | ||
| return 1 | ||
| # rev1 is shorter than rev2 but otherwise equal, hence lesser | ||
| # ...except for goddamn tildes | ||
| if second_list[len(first_list)][0][0] == "~": | ||
| # FIXME: bug?? we return -1 in both cases | ||
| # FIXME: first_list[len(second_list)] should be a string, why are we indexing to 0 twice? | ||
| if second_list[len(first_list)][0][0] == "~": # type: ignore | ||
| return -1 | ||
| return -1 |
There was a problem hiding this comment.
type: ignore due to the str | int ambiguity -- and this is weird and potentially buggy so I'd like to save further changes for a separate PR (unit test coverage of these comparison functions is spotty)
| def __eq__(self, other) -> bool: | ||
| def __eq__(self, other: object) -> bool: | ||
| """Equality magic method impl.""" | ||
| if not isinstance(other, Version): | ||
| return False | ||
| return self._compare_version(other) == 0 |
There was a problem hiding this comment.
Technically a behaviour change, so maybe this should be dropped -- maybe it's better to get an AttributeError when you try to compare a Version for equality with something else?
| @@ -748,8 +777,6 @@ def add_package( | |||
| update() | |||
| cache_refreshed = True | |||
|
|
|||
| packages = {"success": [], "retry": [], "failed": []} | |||
There was a problem hiding this comment.
I separate these into individual variables for better typing
| @@ -748,8 +777,6 @@ def add_package( | |||
| update() | |||
| cache_refreshed = True | |||
|
|
|||
| packages = {"success": [], "retry": [], "failed": []} | |||
Enable static analysis of
apt.pyin ci.LIBPATCHwill be bumped after one more follow up PR handlingrufflinting for this lib.