feat: Add mypy to CI

[last-partizan]

Refs #562

[francoisfreitag]

Good to see such a small patch :)

Mypy needs to be added to the CI (.github/workflows/check.yml), otherwise it won’t run.

[last-partizan]

I'm converting this to a Draft, becouse mypy does not recognize fields and treats them as Any.

And that's the reason it was passing in my local run.

I need to ivestigate this.

[last-partizan]

I figured it out.

By default mypy does not check untyped functions. Only those with explicitly defined types are checked.

If run with --check-untyped-defs it will find 24 errors in 5 files (checked 17 source files)

Well, i think we can start with this small working merge, and see if defining proper types helps to lower error count :)

[francoisfreitag]

Regarding the failure on CI, I believe we can’t skip_install with tox. setuptools_scm generates the version.py, which is imported by phonenumber_field.__init__.

[francoisfreitag]

Thanks for the last improvements. :)

I added a few commits, and would be happy to get your opinion on the following changes:

• enabled check-untyped-defs. That required dropping the types coverage for tests/, but the test code does not benefit as much from being typed, and at least we get some value from mypy.
• use pyproject.toml, to keep the configuration in a single place.
• unpin dependencies, so that newer tools are always used. That simplify maintenance by removing some update to the pinned version, to the cost of sometimes breakage on main.

[francoisfreitag]

The next step would probably be to add strict = true to the configuration (and the corresponding type annotations).

[last-partizan]

enabled check-untyped-defs. That required dropping the types coverage for tests/, but the test code does not benefit as much from being typed, and at least we get some value from mypy.

I don't see check_untyped_defs = true in config, by default it is disabled.

For the start it's fine to leave check_untyped_defs disabled, but with preserved comment that it is disabled, and untyped code is not checked. It was surprize to me that it is disabled by default.

And i think it's better to type-check tests, because that's how library is used, and we will see what errors users will have when using it. With check_untyped_defs disabled they will not produce much errors anyway.

tests/tests.py: note: In member "test_save_change_to_invalid_number_works" of class "PhoneNumberFieldAppTest":
tests/tests.py:328: error: "str" has no attribute "raw_input"  [attr-defined]
            self.assertEqual(tm.phone.raw_input, "invalid")
                             ^~~~~~~~~~~~~~~~~~

For example, this looks like false positive, but it isn't. Type checker recognizes phone as str, because we haven't added types to __new__ constructor.

If you want to enable it and leave tests without type-checking, it's fine. After i add types for PhoneNumberField, we can enable type checking for tests.

Using pyproject.toml and unpinning dependencies is fine.

[francoisfreitag]

Thanks for the input! I mistakenly took check_untyped_defs default to be true, but it isn’t. I introduced it back.

I understand your point about running type verification on the tests as well, as a verifier of types. I restored the check, with --no-check-untyped-defs, so that check_untyped_defs remain enabled for the distributed code, and by default. 👍

I tried typing some tests yesterday, and tripped on the following test.

django-phonenumber-field/tests/test_phonenumber.py

Lines 45 to 56 in 67fae02

	def test_ordering_contains_None(self):
	phone1 = PhoneNumber.from_string("+33600000000")
	msg = "'<' not supported between instances of 'PhoneNumber' and 'NoneType'"
	for assertion in [
	self.assertLess,
	self.assertLessEqual,
	self.assertGreater,
	self.assertGreaterEqual,
	]:
	with self.subTest(assertion):
	with self.assertRaisesMessage(TypeError, msg):
	assertion(phone1, None)

I figured I would need to type the assertions (or use # type ignore), and after a couple minute gave up. How would you address it?

[last-partizan]

I think # type: ignore for this is best solution right now.

There's issue in mypy:
python/mypy#9527

And you can use case(list[Callable[..., None]], assertions) here, but that would be code to satisfy bad type-cheker behaviour.

With pyright for this case we could use tuple instead of list, and it would infer types for all elements like here:

But it's better to leave # type: ignore, and add --warn-unused-ignores to remove them when issue is fixed.

[francoisfreitag]

Awesome, thanks for this investigation 😀

[francoisfreitag]

from Imgflip Meme Generator

[francoisfreitag]

I’ll let this PR sit for a couple days, in case others want to look at it.
Without opinions, I’ll squash and merge it early next week.