[MAINTENANCE] mypy 1.9 + begin wider tests type-checking
#9678
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #9678 +/- ##
========================================
Coverage 82.55% 82.55%
========================================
Files 511 511
Lines 46381 46381
========================================
Hits 38288 38288
Misses 8093 8093 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| import pytest | ||
| import requirements as rp | ||
|
|
||
| IGNORE_PINS: Final[set[str]] = {"mypy", "ruff", "pytest"} |
There was a problem hiding this comment.
It doesn't make sense to check some of these development only packages. These packages SHOULD be pinned, but these checks end up punishing pinning and make it very tedious and error-prone to update the versions/pins.
I've added this IGNORE_PINS constant so that the packaging step just ignores anything here.
I've also had it completely ignore the requirement-types package since most of those types are either stubs or packages that are pulled from other requirement files which are already checked here.
Kilo59
changed the title
mypy 1.9mypy 1.9 + begin wider tests type-checking
Kilo59
force-pushed
the
m/_/test_type-checking
branch
from
cd8ef33
to
0e5d8f1
Compare
| 'tests/actions/test_core_actions\.py', | ||
| 'tests/checkpoint/test_checkpoint_result_format\.py', |
There was a problem hiding this comment.
All these new exclude lines were incredibly tedious to add.
I should have just written a script to do it, but I decided I was already in too deep doing it manually. 😆
| @@ -3355,7 +3351,7 @@ def empty_base_data_context_in_cloud_mode_custom_base_url( | |||
| ) -> CloudDataContext: | |||
| project_path = tmp_path / "empty_data_context" | |||
| project_path.mkdir() | |||
| project_path = str(project_path) | |||
| project_path = str(project_path) # type: ignore[assignment] | |||
There was a problem hiding this comment.
Non-blocking since this isn't actively changing anything, but changing the type of project_path feels bad. I think the best solution here would be just to update line 3362 to be context_root_dir=str(project_path), since it looks like the string version is only used there.
There was a problem hiding this comment.
I agree.
I want to followup with a PR to actually fix the typing issues in conftest.py since it's a core file that almost every other test relies on.
I'm trying to get this one in quickly since new v1 related PR's that merge into develop will almost certainly require changes to the exclude list or the existing type-ignores.
| @@ -7556,21 +7552,22 @@ def _multibatch_generic_csv_generator( | |||
| 5: "category5", | |||
| 6: "category6", | |||
| } | |||
| for batch_num in range(num_event_batches): | |||
| for batch_num in range(num_event_batches): # type: ignore[arg-type] | |||
There was a problem hiding this comment.
I'm guessing reording the params and making the num_ params non-optional is a larger lift? Might be cleanest to just assert instance on them at the top of the method. But that's totally non-blocking.
This PR does a couple different type-checking related things.
mypyto1.9and removes a couple of now unusedtype: ignorecomments because of better type inference.tests. Previously, the only tests being checked weretests/datasource/fluent. In particular, I tried to include tests under active development for thev1release. Also new test files should now be type-checked by default unless they are excluded.test_packagingto make it easier to update pins/versions for things likeruff, &mypyand other development only packages that SHOULD be pinned.I'm happy to split this PR up if desired.