vision icon indicating copy to clipboard operation
vision copied to clipboard

Add missing None type hint to init functions

Open ProGamerGov opened this issue 3 years ago • 6 comments

All __init__ functions should have the type hint -> None according to PEP-0484: https://www.python.org/dev/peps/pep-0484/

ProGamerGov avatar Aug 02 '22 20:08 ProGamerGov

Should I get back and work on #4630 #4612 #4599 ?

oke-aditya avatar Aug 03 '22 05:08 oke-aditya

Thanks for the PR @ProGamerGov . I'm not too keen on merging this one, because simply adding -> None to most methods / functions doesn't really add a lot of value to the code base, it's just extra characters.

As such many types are missing sweat In other files. I just added comment as we can atleast fix up the init in this PR.

Maybe I should get back and try fixing the typing PRs?

cc @datumbox @NicolasHug ?

These typing PRs add very little value to the code-base, and even sometimes negative values. Considering how much time consuming it is to write, review, and then maintain them, I'd recommend spending time on more impactful areas.

NicolasHug avatar Aug 03 '22 08:08 NicolasHug

Yeah I know typing is kind of always a debate. I somehow don't agree to having untyped code as investment in code quality and health is always worth while.

oke-aditya avatar Aug 03 '22 08:08 oke-aditya

investment in code quality and health is always worth while.

Fully agreed. Good docs + good tests cover 90% of that already, for half of the effort it takes to work in an annotated and type-checked code-base.

NicolasHug avatar Aug 03 '22 08:08 NicolasHug

Of course I agree to this.

Considering how much time consuming it is to write, review, and then maintain them, I'd recommend spending time on more impactful areas.

oke-aditya avatar Aug 03 '22 08:08 oke-aditya

I had a look yesterday on the PR to confirm the only changes were the addition of -> None. Though I am usually in favour of typing annotations on methods to clarify the intended input, I too agree that this PR generates too many changes that don't add much value. PEP8 indeed suggests that constructors should be annotated with what you added but some of your changes caused mypy issues which you had to rollback at f5af5abced9e747f6b728fe93a2ab45192c250cc. So given our current setup, you weren't even able to make the suggested change in all places of the code without having the mypy complaining. For this reason, I am also hesitant to approve the PR but I'm not going to block if someone else wants to do it.

datumbox avatar Aug 03 '22 09:08 datumbox