implement Ruff #9
Conversation
There was a problem hiding this comment.
Thank you for all this polish, @bandophahita!
I had a few very minor suggestions that are not blocking.
| from screenpy.exceptions import UnableToAnswer | ||
| from screenpy.pacing import beat | ||
|
|
||
| from ..abilities import MakeAPIRequests | ||
|
|
||
| if TYPE_CHECKING: | ||
| from screenpy import Actor | ||
|
|
||
| subscripts = Union[str, int, slice] |
There was a problem hiding this comment.
We can move this (and the from typing import Union) into the if TYPE_CHECKING block.
There was a problem hiding this comment.
Moved the alias. I let ruff automatically handle the imports.
I noticed it doesn't bother moving the union down into the TYPE_CHECKING block when other items are being imported from the module. My guess is it doesn't save anything.
There was a problem hiding this comment.
Yeah, the main point of moving project code for type hinting into the if TYPE_CHECKING block is to reduce the chance of circular imports caused by type hints, and importing from typing will never cause that.
But i kinda like just moving all the type hinting stuff into that block, since we're using it anyway. I dunno, personal preference.
| from typing import MutableMapping | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING, MutableMapping |
There was a problem hiding this comment.
We can move the MutableMapping into the if TYPE_CHECKING block.
There was a problem hiding this comment.
Same deal here as the Union. I'm guessing ruff sees this as a no-op move so it opts to leave it alone.
tests/test_actions.py
Outdated
| test_method = "TEST" | ||
| test_method: str = "TESTYTEST" |
There was a problem hiding this comment.
Was this change really necessary? Weird.
There was a problem hiding this comment.
I opted to use an uncommon (guaranteed not to be present in the docstring) value; lowering the chance of a false positive.
There was a problem hiding this comment.
Sorry, i should have been more specific—was adding : str necessary? I was OK with the string change, it's funny.
There was a problem hiding this comment.
Oops. I didn't need to leave the annotation in.
I was trying to fix a problem with mypy not liking assert test_method in SendTESTMethod.__doc__ because __doc__ can be str | None and str in None is invalid.
Once I added the assertion assert SendTESTMethod.__doc__ is not None mypy calmed down. I just forgot to take the annotation off.
|
I updated the ruff config to push as many imports into TYPE_CHECKING as it can. |
There was a problem hiding this comment.
A few comments of support, looks good! Thanks for getting this in @bandophahita!
| self.headers = dict(header_pairs[0]) # type: ignore | ||
| self.headers = dict(cast(Iterable, header_pairs[0])) |
There was a problem hiding this comment.
Trust us mypy we know what we're talkin' about here.
| return response | ||
| except JSONDecodeError: | ||
| response = responses[-1].text | ||
| for part in self.body_parts: | ||
| response = response[part] | ||
| return response | ||
| return response |
There was a problem hiding this comment.
Man, i really should have noticed this one. Hahaha. Thanks, linting!
There was a problem hiding this comment.
I removed setup.py from screenpy_playwright and everything worked... maybe it's time to delete this one in the "Project standardization" PR?
No description provided.