run mypy in proper environment

[dimbleby]

Experimenting with running mypy in the proper environment via the github workflow, rather than precommit.

(Or, perhaps, in addition to precommit... for discussion)

As this should show, if everything works as expected, the fact that precommit runs in an environment that does not include most of poetry's dependencies is causing mypy to fail to report lots of errors.

[dimbleby]

Well indeed that makes the point. The typing of this project is nowhere near as clean as precommit suggests.

I invite discussion on what the right approach is.

In the mean time, I will push a second commit to this MR that ignores enough of these errors to go green again. At least we're then explicit about the typing being faulty.

[eyllanesc]

If the feature is going to be implemented then I would recommend that it should be done when poetry fully supports mypy.

[dimbleby]

If the feature is going to be implemented then I would recommend that it should be done when poetry fully supports mypy.

Could you explain what your thinking is?

The intention at #4510 was to make it possible incrementally to improve the typing of this project: by enabling typechecking, then gradually fixing the errors and un-ignoring them. That seems very sensible.

Do you mean that you think we should not turn on typechecking before the project is fully clean? Perhaps I have misunderstood.

[eyllanesc]

@dimbleby

I put myself on the side of the contributor. Let's say I implement some feature in a new file using an existing class that doesn't have typing enabled or has typing errors so my new class then I have 2 options: 1. Add to override list or 2. Fix the problem of the existing class for the github workflows to pass. Although thinking about it, I don't see anything wrong with it.

Since I changed my mind I recommend using the pre-commit and running not only mypy but the other tasks.

[radoering]

Once again we had a platform specific mypy failure in master (see #5524) that would have been revealed by this PR. I suppose the more mypy checking we are doing the more often such issues will occur.

Although running against all platform/python combinations may seem a bit overkill, it's the most accurate way and since combinations run in parallel it shouldn't affect the overall time to wait for CI too much. I don't know if we need to budget CI time, do we? If we don't, I'd just say go for it.

As for the pre-commit hook, of course it's nice to get early feedback from a pre-commit hook but pre-commit passing does not mean CI will pass. If you don't run tests (strictly speaking on all platforms with all python versions) locally you only get the feedback later in CI. With mypy it's just the same. I'd say better to have thorough mypy checks in CI than a mediocre mypy pre-commit hook. So let's get rid of it. (When removing the pre-commit hook, we should add a statement about running mypy in the contribution guide).

In my opinion, we could have already benefited from this PR. I may miss something but I don't see any significant drawbacks.

@abn @finswimmer Any strong feelings against this PR?

[dimbleby]

I have quite a few typechecking MRs open (I know I'm not the only one), I'd love to see them getting merged.

There are two competing / cooperating things going on: MRs to make the typechecking stricter, and MRs to fix the errors

On the making things stricter front:

• this one tightens things up in poetry, we should do the equivalent in poetry-core (though I can report that this will not currently reveal anything new)
• Adding py.typed in both projects is desirable, IMO the typing has gotten far enough that this is now worthwhile
• Not yet started, but it would be good to turn up the mypy strictness flags in this project (I am satisfied that they are already strict in poetry-core). That will reveal a whole new raft of errors, perhaps we should first fix in the current less-strict mode and then repeat the ratchet exercise to gradually clean everything up in more-strict mode.

On the fixing things front: well, there are several MRs listed in #5017. All are as far as I know ready to go, the most relevant right now is #5485 which fixes many of the errors revealed here.

[dimbleby]

rebase post #5485 - note that we no longer need to add additional mypy exceptions to pyproject.toml

[radoering]

@dimbleby It seems we might find consensus when running mypy via pytest-mypy. At first glance, it just looks as good to me as running mypy directly. Do you feel like trying this? I'd appreciate your opinion.

[dimbleby]

Looks as though pytest-mypy doesn't Just Work as a drop-in replacement, though perhaps configuration settings could be found to make it do so. Eg it is hooking into pytest's autodiscovery of files rather than respecting the configuration in pyproject.toml.

Also any error messages get mixed up with the test script runs which I at least find confusing.

Why is this better than running mypy directly?

[radoering]

In my personal opinion it is not. There are some concerns that it's an additional step contributors have to perform before committing (before: pytest + pre-commit, now: pytest + mypy + pre-commit).

Nevertheless, I have the go from @abn that we can proceed with this PR as is and solve that later. Just one question and two requests:

• You've configured mypy to only run on src, not on test. I suppose there are still issues in test that have to be resolved? (Can be done later in a separate PR.)
• Can you please adapt the contribution guide so it says to run mypy in addition to pytest and pre-commit?
• Probably, we should remove the mypy pre-commit hook, shouldn't we?

[dimbleby]

I suppose there are still issues in test that have to be resolved?

Yes. I consider the typechecking of test scripts to be very low priority; but if others care to come along and make it happen then I've no objection.

Pushed updates to the contribution guide and to remove the pre-commit hook.

Consensus about doing it like that for now, pre-commit hook removed

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.