Skip to content
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

Partial adding types to utils folder #5338

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Mar 1, 2024

Should decrease mypy errors by ~200/300

I found some possibly invalid types and I marked this with TODO MYPY so this needs to be fixed in different PR

@danieljohnson2
Copy link
Contributor

@strycore is not fond of all the Optional[str] stuff, so I turned on implicit_optional so we can do optional parameters without it; p: str = None is understood as p: Optional[str] = None.

I see you've put accurate types on all the parameters like that, which is more correct in a way, but noisier.

@qarmin
Copy link
Contributor Author

qarmin commented Mar 2, 2024

I started adding them out of habit.

I'm not sure, but adding them rather allowed more errors to be displayed in mypy(but it is also possible that adding the types to all the parameters allowed mypy to find them better)

I may change them back to var: type = None, but this seems less readable to me than the current solution

@strycore
Copy link
Member

strycore commented Mar 4, 2024

It's fine to have some Optional stuff. Maybe if we have too many of them, it's a sign we should write better, more deterministic code.

@strycore
Copy link
Member

strycore commented Apr 9, 2024

damn, I was about to merge this and there are so many conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants