This PR might be a good candidate for .git-blame-ignore-revs

Codecov Report

Attention: Patch coverage is 96.20253% with 33 lines in your changes missing coverage. Please review.

Project coverage is 97.49%. Comparing base (60be630) to head (57a1db1).
Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main    #6795   +/-   ##
=======================================
  Coverage   97.49%   97.49%           
=======================================
  Files         143      143           
  Lines       28178    28178           
=======================================
  Hits        27473    27473           
  Misses        705      705           

• Flaky Tests Detection - Detect and resolve failed and flaky tests

🚀 Deployed on https://673c121400b2dcb204d7e5e5--pyvista-dev.netlify.app

Thanks for the heads-up @tkoyama010.

@user27182 will this affect development going forward?

Yes, all code will be type-checked after this PR (previously, only code that used annotations explicitly was checked).

Will it e.g. tie our hands to force type hints in all new code?

No, type hints will not be required. Type hints will always be optional. Mypy is usually very good at inferring type on its own without any explicit annotations. But, if you push unannotated code that is not type-safe, mypy will now complain, whereas previously it would not have. Even then, you aren't forced to make any actual changes to code, but in this case you will now need to type: ignore the errors (just like the errors ignored by this PR).

So, this PR forces all code to be type-checked. It does not force code to be annotated. It's up to devs if they want to resolve any errors or if they'd rather just ignore them.

So, this PR forces all code to be type-checked. It does not force code to be annotated. It's up to devs if they want to resolve any errors or if they'd rather just ignore them.

Arguably type: ignore or Any is just a lazy work-around. So then it would indeed be a strong push to type hint things.

Arguably type: ignore or Any is just a lazy work-around. So then it would indeed be a strong push to type hint things.

I disagree with this. Once again, this PR has little to do with type hints, it's about type checks.

Type hints are not the solution to type errors. If I do x = 1 the type is inferred as int. If I later do x = 1.0 this is a type error because a float is assigned instead of an int. This could easily be a bug, especially when you replace 1 and 1.0 with function calls (which may return int or float or something else altogether).

So the solution in this case is to ignore it (because I wrote unit tests for this explicitly and know what I'm doing), or fix the cause of the type error by modifying the code (e.g. use a different variable, call a different function, whatever).

So, in my view the statement should read more like:

Arguably type: ignore or Any is just a lazy work-around. So then it would indeed be a strong push to type hint things reduce bugs through type-checking.

Will it e.g. tie our hands to force type hints in all new code?

For reference, the option that does force using type hints is disallow_untyped_defs. This would be a separate PR, and is the option which I believe would get more push-back. I don't plan on proposing adding this option though.

I am softly in favor of adding this PR given that we are already typing in many places. It increases typing burden by increasing type checking coverage, not adding in extra typing restrictions. Another possible path to get to this flag turned on is to try to clean up as many problems up front (getting to 100% seems unlikely), but I think they will be hard to find.

A reason I am in favor of this PR because we could open an issue as 'good first issue' to remove typing ignore lines. While some typing problems will be quite difficult to resolve, there are some in a quick read through that are relatively straightforward to resolve if someone is familiar with typing and not familiar with pyvista, e.g. initializing a variable as None and then reassigning it to another type later.

A reason I am in favor of this PR because we could open an issue as 'good first issue' to remove typing ignore lines. While some typing problems will be quite difficult to resolve, there are some in a quick read through that are relatively straightforward to resolve if someone is familiar with typing and not familiar with pyvista, e.g. initializing a variable as None and then reassigning it to another type later.

To add to this, I find that it is generally overwhelming at the moment to try to resolve typing issues. Part of the reason for this is that, if you even add a single annotation to a function that currently has zero annotations, this implicitly "turns on" type-checking for that function, and then suddenly all of the existing type errors related to that function are reported. So, even just trying to add a return type to a function becomes a rabbit hole of resolving previously unreported type errors. This is very off-putting and can prevent devs from even trying to fix type errors.

But, if we ignore these errors up-front and turn on type-checking for all code (this PR), this makes it a lot easier to do small, iterative fixes later. There are also tools like autotyping which we can leverage to automate a lot of "obvious" cases that are implicitly known by type-checkers. But, we can't currently use these tools for the reason above, since adding these automated annotations will cause mypy to also complain about any number of the existing largely unrelated 1000+ type errors.

As an example, if I want to automatically add a None return type for functions in core, I can do python -m autotyping pyvista/core --none-return. If I do this on the main branch and run mypy, I get 52 errors to resolve. But if I do that from this branch, I only get 11 errors, which is much more manageable to resolve quickly. Plus, the 11 errors are now known to be directly related to None return types. It was then relatively easy for me to create #6802 and resolve the errors.

This PR might be a good candidate for .git-blame-ignore-revs

Absolutely.

Part of the reason for this is that, if you even add a single annotation to a function that currently has zero annotations, this implicitly "turns on" type-checking for that function, and then suddenly all of the existing type errors related to that function are reported.

Yep, encountered this in one of my private codebases as well. Only solution was a massive and overwhelming PR to do it in one go, ignore all the pyvista issues, and then merge it hoping that I wouldn't regret it later. This PR proposes a much better approach because we can incrementally add typechecking rather than being forced to do it all at once while enabling mypy.

Will it e.g. tie our hands to force type hints in all new code?

In my opinion, we should enforce type hints. It's the best (as far as I can tell) programmatic way without unit tests to enforce code safety. I've uncovered a mountain of issues that could have only been resolved through unreasonable unit tests or simply resolving user errors. Typehinting is the way to go, especially since our library is used downstream by so many applications.

Also:

we could open an issue as 'good first issue' to remove typing ignore lines

100% agree with this. Request users take a look at all type: ignore and remove them, adding types along the way to be in-compliance with mypy.

Thanks for everyone's feedback and support getting this approved!

@tkoyama010 I'm thinking it's possible that some of the other None annotation PRs which are already queued to merge may cause a conflict with this PR (and require re-approval). Is it better to let this PR merge first so that conflicts may be resolved through the other PRs instead of through this one?

Thanks for everyone's feedback and support getting this approved!

@tkoyama010 I'm thinking it's possible that some of the other None annotation PRs which are already queued to merge may cause a conflict with this PR (and require re-approval). Is it better to let this PR merge first so that conflicts may be resolved through the other PRs instead of through this one?

Thank you for pointing out. We will merge this first.