chore: Enhance type checking with mypy and improve code quality #285
Conversation
cdragos
force-pushed
the
chore/pyright
branch
3 times, most recently
from
2c93bd9 to
09eb72d
Compare
cdragos
changed the title
|
Why pyright instead of mypy? I don't have strong opinions on which one is better, but I don't like to change tools without good reasons. I'm happy we now have type checking, but I don't think changing tools will improve it a lot. I prefer to keep iterating on mypy config and enable many options still disabled. |
|
@iurisilvio Thanks for your feedback. I apologize for not providing a description earlier. I agree about being cautious with changing tools. While exploring
One feature I found particularly useful is pyright's Inferred Return Types, similar to TypeScript. Unlike Many more differences documented here made me think To move forward, I propose the following alternatives:
Let me know what you think! |
|
I prefer to not add a new tool now. There is a lot of work improving mypy behaviour and another tool will split efforts with small gains. Can you go with option 2? |
roboflow/__init__.py
Outdated
| @@ -282,7 +275,7 @@ def project(self, project_name, the_workspace=None): | |||
|
|
|||
| dataset_info = dataset_info.json()["project"] | |||
|
|
|||
| return Project(self.api_key, dataset_info) | |||
| return Project(self.api_key or "", dataset_info) | |||
There was a problem hiding this comment.
I don't think this is the fix. api_key could be None for public projects.
I prefer to ignore the error instead of fix the type and cover a possible bug.
roboflow/models/video.py
Outdated
| @@ -65,7 +65,7 @@ def __init__( | |||
| """ # noqa: E501 // docs | |||
| self.__api_key = api_key | |||
|
|
|||
| def predict( # type: ignore[override] | |||
| def predict( | |||
There was a problem hiding this comment.
This is an example where mypy do better. This override with incompatible signature is bad.
I don't know how to fix it without breaking compatibility, but I like to have it annotated as an issue.
There was a problem hiding this comment.
Actually, I disabled this from pyproject.toml. Pyright detected it with the standard configuration.
| @@ -59,20 +59,13 @@ def check_key(api_key, model, notebook, num_retries=0): | |||
| return "onboarding" | |||
|
|
|||
|
|
|||
| def auth(api_key): | |||
There was a problem hiding this comment.
It is backward incompatible. I'd prefer to keep this function.
There was a problem hiding this comment.
Okay, but I am curious about the use case, at this moment the calling of this function raises an exception
TypeError: auth() missing 1 required positional argument: 'api_key'
There was a problem hiding this comment.
Ah ok , I didn't realized it is totally broken. 👍🏻 Yes, we can remove it.
There was a problem hiding this comment.
Can you remove it again? 😅 I reviewed all the PR already (all good), I'll wait for this last change.
cdragos
changed the title
|
@iurisilvio Thank you for your thorough feedback and guidance throughout this PR process. I've addressed all the comments raised. 🏄🏼 |
|
Thanks! 🚢 |
This PR enhances our code quality checks and type safety:
check_code_qualityprocesspathlib.Pathfor better cross-platform support.dockerignore