-
Notifications
You must be signed in to change notification settings - Fork 3
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
Consistently use Python 3.10 type annotations #263
Changes from all commits
52ac4d8
91e4928
c728181
80d3cf7
4608122
b6d115b
e2d0481
3ec24af
365e8ce
d22f606
ec6eab3
c8a84f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
from __future__ import annotations | ||
|
||
import abc | ||
from dataclasses import dataclass, field | ||
from functools import lru_cache | ||
from typing import Any, Callable, Collection, List, Optional, Tuple, TypeVar, Union | ||
from typing import Any, Callable, Collection, List, Optional, TypeVar | ||
|
||
import sqlalchemy as sa | ||
|
||
|
@@ -16,7 +18,7 @@ | |
ToleranceGetter = Callable[[sa.engine.Engine], float] | ||
|
||
|
||
def uncommon_substrings(string1: str, string2: str) -> Tuple[str, str]: | ||
def uncommon_substrings(string1: str, string2: str) -> tuple[str, str]: | ||
qualifiers1 = string1.split(".") | ||
qualifiers2 = string2.split(".") | ||
if qualifiers1[0] != qualifiers2[0]: | ||
|
@@ -29,29 +31,29 @@ def uncommon_substrings(string1: str, string2: str) -> Tuple[str, str]: | |
@dataclass(frozen=True) | ||
class TestResult: | ||
outcome: bool | ||
_failure_message: Optional[str] = field(default=None, repr=False) | ||
_constraint_description: Optional[str] = field(default=None, repr=False) | ||
_factual_queries: Optional[str] = field(default=None, repr=False) | ||
_target_queries: Optional[str] = field(default=None, repr=False) | ||
_failure_message: str | None = field(default=None, repr=False) | ||
_constraint_description: str | None = field(default=None, repr=False) | ||
_factual_queries: str | None = field(default=None, repr=False) | ||
_target_queries: str | None = field(default=None, repr=False) | ||
|
||
def formatted_failure_message(self, formatter: Formatter) -> Optional[str]: | ||
def formatted_failure_message(self, formatter: Formatter) -> str | None: | ||
return ( | ||
formatter.fmt_str(self._failure_message) if self._failure_message else None | ||
) | ||
|
||
def formatted_constraint_description(self, formatter: Formatter) -> Optional[str]: | ||
def formatted_constraint_description(self, formatter: Formatter) -> str | None: | ||
return ( | ||
formatter.fmt_str(self._constraint_description) | ||
if self._constraint_description | ||
else None | ||
) | ||
|
||
@property | ||
def failure_message(self) -> Optional[str]: | ||
def failure_message(self) -> str | None: | ||
return self.formatted_failure_message(DEFAULT_FORMATTER) | ||
|
||
@property | ||
def constraint_description(self) -> Optional[str]: | ||
def constraint_description(self) -> str | None: | ||
return self.formatted_constraint_description(DEFAULT_FORMATTER) | ||
|
||
@property | ||
|
@@ -121,12 +123,12 @@ def __init__( | |
self, | ||
ref: DataReference, | ||
*, | ||
ref2: Optional[DataReference] = None, | ||
ref_value: Optional[Any] = None, | ||
name: Optional[str] = None, | ||
output_processors: Optional[ | ||
Union[OutputProcessor, List[OutputProcessor]] | ||
] = output_processor_limit, | ||
ref2: DataReference | None = None, | ||
ref_value: Any = None, | ||
name: str | None = None, | ||
output_processors: OutputProcessor | ||
| list[OutputProcessor] | ||
| None = output_processor_limit, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why a None did sneak in here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this change, If we convert the If we now also covert the Does that make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems I missed a step in the chain of transformations. |
||
cache_size=None, | ||
): | ||
self._check_if_valid_between_or_within(ref2, ref_value) | ||
|
@@ -136,8 +138,8 @@ def __init__( | |
self.name = name | ||
self.factual_selections: OptionalSelections = None | ||
self.target_selections: OptionalSelections = None | ||
self.factual_queries: Optional[List[str]] = None | ||
self.target_queries: Optional[List[str]] = None | ||
self.factual_queries: list[str] | None = None | ||
self.target_queries: list[str] | None = None | ||
|
||
if (output_processors is not None) and ( | ||
not isinstance(output_processors, list) | ||
|
@@ -156,7 +158,9 @@ def _setup_caching(self): | |
self.get_target_value = lru_cache(self.cache_size)(self.get_target_value) # type: ignore[method-assign] | ||
|
||
def _check_if_valid_between_or_within( | ||
self, ref2: Optional[DataReference], ref_value: Optional[Any] | ||
self, | ||
ref2: DataReference | None, | ||
ref_value: Any, | ||
): | ||
"""Check whether exactly one of ref2 and ref_value arguments have been used.""" | ||
class_name = self.__class__.__name__ | ||
|
@@ -228,13 +232,11 @@ def condition_string(self) -> str: | |
|
||
def retrieve( | ||
self, engine: sa.engine.Engine, ref: DataReference | ||
) -> Tuple[Any, OptionalSelections]: | ||
) -> tuple[Any, OptionalSelections]: | ||
"""Retrieve the value of interest for a DataReference from database.""" | ||
pass | ||
|
||
def compare( | ||
self, value_factual: Any, value_target: Any | ||
) -> Tuple[bool, Optional[str]]: | ||
def compare(self, value_factual: Any, value_target: Any) -> tuple[bool, str | None]: | ||
pass | ||
|
||
def test(self, engine: sa.engine.Engine) -> TestResult: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical, but the change from optional any to any feels wrong. But probably is an issue that Any is too vague to start with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. Do you suggestion yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no suggestion. Well, one, but it's quite vague. To create a type that reflects what a
ref_value
can be. But it feels too much work for little gain.