Update syntax for existing type annotations

[pierwill]

Status

Ready for review

Description of Changes

Update syntax for existing type annotations.

Should fix #5282.

[lgtm-com]

This pull request introduces 1 alert when merging b1f28a5 into e300a6e - view on LGTM.com

new alerts:

• 1 for Syntax error

[pierwill]

Not sure what's holding up CI here...

[kushaldas]

After digging more into this I think pylint is not ready yet for type annotations :(

Even after I bring in all the imports into the main code from the if typing.TYPE_CHECKING condition, pylint is still failing.

Related similar issues:

• Add option to ignore code in if TYPE_CHECKING pylint-dev/pylint#3285
• Pylint reports ungrouped-imports for TYPE_CHECKING imports pylint-dev/pylint#3382

We should discuss about this during the standup tonight.

@pierwill Please hold of any future PR like this until we figure out a way. Otherwise pylint will keep failing them.

[redshiftzero]

It makes sense to migrate away from type comments now that we're on Python 3. To address the lint failures we have the following options:

1. Not use pylint. Bad because it's a pretty useful tool.
2. Use pylint but litter the code with noqa exceptions. This is ugly.
3. Use pylint but add a global exception for E0601 / used-before-assignment which is causing the issue. Bad because it might cause bugs to be missed in the future for that specific lint rule.
4. Add imports needed for type checking to the imports at runtime and combine them to sidestep the other lint error @kushaldas found. Not a bad option but it means that it's unclear which imports are needed at runtime and which are needed solely for type checking, as well as obviously that there would be unnecessary imports at runtime.
5. For the imports that are only performed when TYPE_CHECKING, we can express the type hint as a string literal (which mypy will still use).

I think option 5 makes the most sense. We can revert this when pylint-dev/pylint#3285 is addressed upstream. See commit d3707e6 which does this on top of this PR and has a passing lint target. Let me know what you think @kushaldas @pierwill !

[kushaldas]

I think option 5 makes the most sense. We can revert this when pylint-dev/pylint#3285 is addressed upstream. See commit d3707e6 which does this on top of this PR and has a passing lint target. Let me know what you think @kushaldas @pierwill !

Let us go with 5. We will revert back when pylint is ready for type hinting.

[rmol]

Counterpoint: we're not even using pylint on our other projects, just flake8, which evidently doesn't have these problems with type annotations. I think it'd be worth reevaluating whether we really need both, instead of letting one linter's shortcomings dictate a different style of annotation to what we're using everywhere else.

[kushaldas]

@pierwill thank you for the work. Now you can finish off the rest of the changes following @redshiftzero.

[redshiftzero]

Hmm fair, I'll file an issue for considering pylint removal, when testing this branch I did find more linting issues in pylint compared to flake8 (worth investigating what the difference is). However, we'll still get F821 lint failures even with flake8 only due to our use of if TYPE_CHECKING which we don't hit in other repos because we do the type-checking imports also at runtime, i.e. we'd still need to use the string literal type annotations. Let me know if you think I'm misunderstanding something.