gtalarico / pyairtable

Python Api Client for Airtable
https://pyairtable.readthedocs.io
MIT License
765 stars 138 forks source link

Run mypy as part of `make lint`, `tox -e lint` #217

Closed mesozoic closed 1 year ago

mesozoic commented 1 year ago

I noticed that mypy isn't run or enforced as any part of the build chain, so I added it to the lint targets. This might make it easier to catch any issues in pull requests in the future. (I've got a couple I'd like to send in soon.)

I couldn't really tell what the pyairtable.compat module was for, but mypy didn't seem to like it (error) and removing it didn't break any of the tests.

gtalarico commented 1 year ago

It's been a while but I think the compat was added as a place to stash logic to handle typing when optional imports are used Retry. https://stackoverflow.com/a/61394121/4411196 I do see the error you mentioned, we need to fix it.

PS: I see you pointed to the Retry under requests. I think the issue was that as a lib we retried only requests >= 2 and Retry is only available in a later 2.x. If we go this route we would need to make sure we require a requests version that includes the vendored Retry lib.

mesozoic commented 1 year ago

That's a good catch, thanks! I bumped up to the earliest version of requests that has a vendored library, which is still a few years old (so presumably few people out there still need older versions). To ensure nobody else makes the same mistake I did, I've added an extra environment specifier to tox.ini that will run tests on both the minimum required version and the latest version of the requests library.

It looks like #182 added a couple other mypy failures (saved here), and I wasn't quite sure what to make of them. I did my best to sort it out, and it looks like there was some unused code that could just be removed. Open to feedback if there's something I missed there, however. @larsakerson FYI

mesozoic commented 1 year ago

Actually, reading https://github.com/python/typeshed/issues/6893, it looks like we can't rely on the vendored version of the urllib3 library once we use the latest version of mypy and type stubs. So I've switched the code to refer directly to urllib3, ~and now the tests pass even farther back (requests==2.9.0)~ (ed: nope, this was wrong). I believe this is ready to merge but still open to things I've missed.

mesozoic commented 1 year ago

I can see the docs build has failed. Bear with me, I'm learning here... will push again once I've figured it out.

mesozoic commented 1 year ago

Looking at this build failure readthedocs.org is still building on Python 3.7, which means we can't use flake8 >= 6.0.0. I've removed the minimum version requirements from linting dependencies. I've also moved the docs build into tox so it's more reproducible.

It looks like the version of Python used for building documentation can be configured but I'm not trying to take that on right now. I did encounter one formatter warning on the existing documentation, which I've fixed.

mesozoic commented 1 year ago

@gtalarico Apologies for the comment spam above, but I've got tests passing, docs building, and GH actions are green. Are you okay with bumping up the minimum requests library to 2.22.0? (It's almost four years old so reasonably safe.)

gtalarico commented 1 year ago

Good with me