freelawproject / bigcases2

The sequel to Big Cases Bot
Other
18 stars 11 forks source link

Adding mypy to the pre-commit hook #522

Closed TheCleric closed 6 months ago

TheCleric commented 7 months ago

I noticed that mypy was not running on the pre-commit hook and a note saying it had been tried before without success. I experimented with what it would take to do this and was successful in getting it working. However I wanted to gauge the interest in adding this, since it comes with a few downsides that I think are important to think through.

Upsides

Downsides

  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.991
    hooks:
     - id: mypy
       additional_dependencies: [
          beautifulsoup4==4.12.2,
          boto3==1.34.74,
          courts-db==0.10.17,
          django-debug-toolbar==4.3.0,
          django-environ==0.11.2,
          django-hcaptcha==0.2.0,
          django-htmx==1.17.3,
          django-rq==2.10.2,
          django-stubs==4.2.0,
          "django-tailwind[reload]==3.8.0",
          "Mastodon.py[cryptography]==1.8.0",
          pillow==10.3.0,
          psycopg2-binary==2.9.6,
          sentry-sdk==1.44.0,
          twitterapi==2.8.2,
          types-requests==2.31.0.1,
        ]
mlissner commented 7 months ago

Where was the note about trying it before? I don't recall what the problem was. That performance is a bit of a bummer, and might be enough of a problem in itself...

TheCleric commented 7 months ago

Where was the note about trying it before? I don't recall what the problem was. That performance is a bit of a bummer, and might be enough of a problem in itself...

At the bottom of .pre-commit-config.yaml:

# Tried and Failed
#
# 1. I tried doing mypy too, but it was a mess because it runs in its own
#   isolated environment, that lacks all our dependencies.
mlissner commented 7 months ago

Got it. That's not horrible, but it's annoying to have to maintain the dependencies properly in both places. On the other hand, it's nice to have mypy running automatically, and I've certainly noticed I have a tendency not to bother with it until it crashes.

I'm +1 on adding it here experimentally. We can see what people think, unless somebody chimes in to be -1 on it.

TheCleric commented 6 months ago

@mlissner just so I'm clear, does that mean to go ahead and submit a PR and get feedback then, or wait for feedback here?

mlissner commented 6 months ago

Yeah, I think that's a good move. I'm +1, nobody else -1'ed it. Big Cases is by design a bit of a playground to see how things go before moving them over to CourtListener, so, yeah, let's go for it!