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

Add mypy to CI #189

Merged
merged 23 commits into from
Feb 11, 2025
Merged

Add mypy to CI #189

merged 23 commits into from
Feb 11, 2025

Conversation

haacked
Copy link
Contributor

@haacked haacked commented Feb 11, 2025

A recent issue #187 noted that a recent PR (#182) introduced new errors when running mypy .. Shout out to @rafaeelaudibert for jumping on a fix (#188).

In the issue, @rafaeelaudibert asked (emphasis mine):

Agh, great catch, let me fix this. I don't understand why CI didn't catch this

Well I found the reason. CI doesn't run mypy. 😆

And for good reason, it would produce almost 700 errors. However, we can try to be good citizens moving forward by not allowing new mypy errors and fix existing ones over time.

So this PR does the following:

  1. Adds additional dev dependencies related to running mypy.
  2. Adds a baseline filter file created with mypy-baseline
  3. Adds mypy . | mypy-baseline filter to the CI. This should only warn on new errors.
  4. Adds a mypy.ini so we can configure the mypy errors we care about.

When someone fixes old mypy errors, they'll need to run mypy . | mypy-baseline sync to generate a new baseline.

Open Question for Reviewers

  • Do we care about all of the mypy warnings? Right now I have them all set to True.
  • I ignored all errors in tests. Seems good for now, but maybe we want some checking in the future?

And configure mypy. However, there are still a ton of errors. Not ready to add this to CI yet.
But only report on new errors
@haacked haacked requested a review from a team February 11, 2025 16:42
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds mypy type checking to the CI pipeline to prevent future type-related issues, addressing a gap where type errors weren't being caught during continuous integration.

  • Added mypy.ini with strict type checking configuration, ignoring test files and third-party imports
  • Created mypy-baseline.txt to track ~700 existing type errors while preventing new ones
  • Added mypy and related type stub packages to dev dependencies in setup.py
  • Integrated mypy . | mypy-baseline filter command in CI workflow to catch only new type errors

4 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

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

Thank you!

I don't have any suggestions on what the setup should look like. We should try to make it look the closest possible to our posthog/posthog project, but I'm fine to just use a separate subset of checks

@haacked
Copy link
Contributor Author

haacked commented Feb 11, 2025

I don't have any suggestions on what the setup should look like. We should try to make it look the closest possible to our posthog/posthog project, but I'm fine to just use a separate subset of checks

Good idea! I updated the mypy.ini here to look like https://github.com/PostHog/posthog/blob/master/mypy.ini.

@haacked haacked merged commit 38683e8 into master Feb 11, 2025
2 checks passed
@haacked haacked deleted the haacked/add-dev-dependencies branch February 11, 2025 17:50
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.

2 participants