fixing mypy errors with loosest rules #2040
Conversation
|
Cool stuff. Will take a real look next week. Do we need to add mypy to the build or would this already fail the build? |
|
There's no need to add mypy as a I guess it's up to you/the project to decide if there should be any type checker, and if so which one, and which rules should be used. This is why I ran with minimal rules, since I didn't want to impose too much. |
|
We can do minmal rules but it would be nice to add mypy to tox.ini (otherwise we would just end up breaking your project and others down the line) |
|
Cool, I'll add mypy to tox.ini and update the PR! I'll see if I get some time during the weekend, otherwise I'll do it beginning next week. |
fixed typing errors in locust/test.
| @@ -301,7 +304,9 @@ def _get_user_current_count(self, user: str) -> int: | |||
|
|
|||
| def _distribute_users( | |||
| self, target_user_count: int | |||
| ) -> Tuple[dict, Generator[str, None, None], typing.Iterator["WorkerNode"], List[Tuple["WorkerNode", str]]]: | |||
| ) -> Tuple[ | |||
| Dict[str, Dict[str, int]], Generator[Optional[str], None, None], itertools.cycle, List[Tuple["WorkerNode", str]] | |||
There was a problem hiding this comment.
Is str really optional? Feels like a different solution could be possible?
There was a problem hiding this comment.
it was due to:
def infinite_cycle_gen(users: List[Tuple[Type[User], int]]) -> itertools.cycle:
if not users:
return itertools.cycle([None])
i made quick check, and change it to itertools.cycle([]) and removed Optional and mypy didn't complain. just about to logout, but i'll run the tests tomorrow to make sure they still pass as well.
There was a problem hiding this comment.
This one was a bit tricky, itertools.cycle([None]) gives you an endless loop of None values, and where these values are used there's checks, like:
while user_count < target_user_count:
user = next(user_gen)
if not user:
break
and,
for user in self._user_generator:
if not user:
self._no_user_to_spawn = True
break
While itertools.cycle([]) gives you nothing, e.g., it would be like for _ in [].
So I would say that yes, it is an Optional[str], otherwise there would have to be quite some logic thas has to be re-written in dispatch.py
There was a problem hiding this comment.
Ok, so this one can be typed as str when using --no-strict-optional settings for mypy. I'd like to see it typed as Optional[str] though :) But your call.
locust/dispatch.py
Outdated
| @@ -62,13 +61,13 @@ def __init__(self, worker_nodes: "List[WorkerNode]", user_classes: List[Type[Use | |||
| assert len(user_classes) > 0 | |||
| assert len(set(self._user_classes)) == len(self._user_classes) | |||
|
|
|||
| self._target_user_count = None | |||
| self._target_user_count: int | |||
There was a problem hiding this comment.
This changes self._target_user_count to be undefined (rather than None), right? Maybe that is ok, but I just want to be clear :)
There was a problem hiding this comment.
Ah crap... if it would be used before actually being set, it would actually throw AttributeError, e.g. it would not be None.
I see 3 options:
- Change them to
Optional[..], and then deal with all the typing errors that it might cause if it's used without checking that it's notNone, e.g. changing this one toOptional[int]produces:
➜ mypy locust/ --ignore-missing-imports
locust/dispatch.py:147: error: Unsupported operand types for < ("int" and "None")
locust/dispatch.py:147: note: Right operand is of type "Optional[int]"
locust/dispatch.py:157: error: Unsupported operand types for > ("int" and "None")
locust/dispatch.py:157: note: Right operand is of type "Optional[int]"
locust/dispatch.py:262: error: Value of type variable "SupportsRichComparisonT" of "min" cannot be "Optional[int]"
locust/dispatch.py:274: error: Unsupported operand types for >= ("int" and "None")
locust/dispatch.py:274: note: Right operand is of type "Optional[int]"
locust/dispatch.py:284: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Optional[int]"
locust/dispatch.py:295: error: Unsupported operand types for <= ("int" and "None")
locust/dispatch.py:295: note: Right operand is of type "Optional[int]"
Found 6 errors in 1 file (checked 61 source files)
- Set an initial value of e.g.
0 - Use
setattrto monkey patch them with an initial value ofNone # type: ignore
I'd say that the list is ordered by, my, preference. Where 1 would cause most changes.
However, all the tests pass, so internally locust does not seem to try to use the variables before they have actually been set.
There was a problem hiding this comment.
Perhaps we could use --no-strict-optional? It would simplify things in a lot of places.
There was a problem hiding this comment.
Or maybe... it seems possible to do self._target_user_count: int = None?
It could definitely be argued that it is weird to have a None-value in something that is not marked as an Optional, but in this case I think it may make sense, because it will not be None for outside users.
Having None-checks all over the place just to keep the type checker happy (option 1) seems excessive (and options 2-4 feel even worse :) (although you could argue that this solution is just a variant of option 4)
There was a problem hiding this comment.
Oh man, I was looking for an option to mypy that didn't force marking variables Optional for having a None value. My bad for missing this one.
The combination that worked as self._target_user_count: int = None and --no-strict-optional parameter to mypy.
There was a problem hiding this comment.
👍 Lets do --no-strict-optional for now, and remove the Optional in declarations (except when it is obvious that it may happen even in normal circumstances and thus need to be handled in user code)
|
Thanks! |
tried to update locust to 2.8 in a project that uses mypy for static type checking and ran into problems.
noticed that
locust/py.typedhad been added (TIL). i've tried to cleanup the typing in locust by running mypy with default settings and fixing the errors.with this PR, the tests succeeds, and:
while testing the changes i had some problems with the packaging when specifying locust dependency from a git repo.
i noticed that
setuptools-scmhas deprecated usingsetup.pyafter version 6.0.1, so i added a constraint for that package insetup.py. (also, theblackconfiguration specified inpyproject.tomlcaused problems withpip, causing it to think it was going to use that instead ofsetup.{py,cfg}, unfortunately the black maintainers won't add support for providing it viasetup.cfg).