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

Restore running mypy in CI #1001

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Restore running mypy in CI #1001

merged 7 commits into from
Dec 16, 2024

Conversation

neob91-close
Copy link
Contributor

@neob91-close neob91-close commented Dec 13, 2024

Main changes:

  • Run mypy in GHA
  • Add specific # type: ignore comments for existing mypy errors

@neob91-close neob91-close changed the title Restore mypy Restore running mypy in CI Dec 13, 2024
@neob91-close neob91-close force-pushed the restore-mypy branch 3 times, most recently from 8d09bb4 to 3cac5ca Compare December 13, 2024 06:57
@tsx
Copy link
Contributor

tsx commented Dec 16, 2024

I'm very meh on the amount of type ignores in this PR. I feel like we should have changed mypy settings instead.

@tsx tsx removed their request for review December 16, 2024 07:33
@neob91-close
Copy link
Contributor Author

In what way would you change the mypy settings?
Do you mean you'd make mypy more lenient so that we could add fewer type ignores?

Looks like we can maybe take the number of type ignores down from ~2000 to ~600 by manipulating the mypy config, but I don't think it's the better approach. I'd prefer stricter type-checking for all new code.

What drawbacks do you see in the number of type ignores?

@tsx
Copy link
Contributor

tsx commented Dec 16, 2024

It's mostly the amount of extra noise.

The two main kinds of errors are:

  • no-untyped-def - do you really need enforcement to keep adding annotations in new code? Ideally we could just annotate the all the existing stuff instead. It will take some effort for sure but I feel like it's very feasible.
  • import-not-found - these are fixed by either updating the library or adding stubs. Sprinkling the codebase with type-ignores will just create more cleanup work when you do the update/stubs.

Copy link
Contributor

@squeaky-pl squeaky-pl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see arguments for going both ways and I am not sure which way we want to go.

Are you going to be adding new code soon, like in next cycles?

I would at least run counts of each type-ignores. Create an issue which lists those in ascending order and put it to on your managers radar as a badly needed tech investment. Then each of type-ignore type can be tackled separately.

@neob91-close
Copy link
Contributor Author

I would at least run counts of each type-ignores. Create an issue which lists those in ascending order and put it to on your managers radar as a badly needed tech investment. Then each of type-ignore type can be tackled separately.

#1002

Are you going to be adding new code soon, like in next cycles?

Not sure, but given that we don't have as much experience working with sync-engine, we'll need all the help we can get.

@neob91-close neob91-close merged commit f6f2371 into master Dec 16, 2024
4 checks passed
@neob91-close neob91-close deleted the restore-mypy branch December 16, 2024 12:35
@neob91-close
Copy link
Contributor Author

FYI @maxpollack

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.

3 participants