Conversation
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against e1b63c4 |
khvn26
force-pushed
the
feat/mypy
branch
4 times, most recently
from
d891f1e to
b2fa987
Compare
khvn26
force-pushed
the
feat/mypy
branch
9 times, most recently
from
4a9ebb8 to
179e079
Compare
novakzaballa
left a comment
novakzaballa
left a comment
There was a problem hiding this comment.
@khvn26 I am approving since in general it LGTM, however, there are a lot of changes, not sure if everything is OK, maybe we need another couple of eyes here?
khvn26
force-pushed
the
feat/mypy
branch
2 times, most recently
from
832de2c to
4fa7ec3
Compare
khvn26
force-pushed
the
feat/mypy
branch
6 times, most recently
from
d7402a9 to
1a44ac9
Compare
khvn26
force-pushed
the
feat/mypy
branch
3 times, most recently
from
c261709 to
4733f50
Compare
- add strict mode mypy, fix typing errors - add absolufy-imports, ditch relative imports - move segment evaluation logic to `flag_engine.segments.evaluation` module - add `type: ignore` comment for decorator usage on a property - add `type: ignore` comments for untyped dependencies
zachaysan
left a comment
zachaysan
left a comment
There was a problem hiding this comment.
Looks good. I have a couple small questions, but no showstoppers.
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
A couple of minor comments / questions but looks good on the whole.
| def inner( | ||
| segment_value: typing.Optional[str], | ||
| trait_value: TraitValue, | ||
| trait_value: typing.Union[TraitValue, semver.Version], |
There was a problem hiding this comment.
Ugh. More trait value pain!
There was a problem hiding this comment.
This one is actually a typing pain to accommodate for L183-L185. Maybe we should actually move semver parsing inside the get_casting_function.
|
|
||
| def get_hashed_percentage_for_object_ids( | ||
| object_ids: typing.Iterable[typing.Any], iterations: int = 1 | ||
| object_ids: typing.Iterable[SupportsStr], iterations: int = 1 |
There was a problem hiding this comment.
This is the broadest correct type I could think of without resorting to either Any or a type: ignore comment.
The "real" type is list[ int | str | UUID ], but that should not matter to the context of this function.
|
|
||
|
|
||
| def remove_semver_suffix(value: str) -> str: | ||
| def remove_semver_suffix(value: semver.Version) -> str: |
There was a problem hiding this comment.
Is this true? I thought we did pass a string in?
There was a problem hiding this comment.
Turns out we never did. All string values suffixed with ":semver" are converted to semver.Version before this function gets called.
4 participants
No description provided.