sopel: type hint and basic setup

[Exirel]

Description

I wanted to add type annotation, and make sure they actually work, i.e. that mypy could validate them. So I installed it, ran it, and... got a load of errors. So tried to fix them, one by one.

Some are easy to fix: for sqlalchemy or pytest, all I had to do was to upgrade the dependency. Given that we are now Py3.6+ only, it was fairly easy and shouldn't be a problem. At least I hope it won't. At least, the test suite is still running fine on my end (Py3.8), and I'm doing this PR to check how it behave on the CI too. If it doesn't work, I'll see what I can do!

Some are trivial to fix: install a type library:

• types-pkg-resources
• types-pytz
• types-requests

Some errors are more complicated, and some are limitation of what is available in Py3.6 (compared to what you can do with 3.8 and 3.9). I don't want to use a backport library yet, because I want to make sure that the very basic typing work properly, one step at a time.

That's also why I didn't put mypy with the make qa command: I really like mypy, but I'd like to wait a bit before we enforce it for everyone. Making sure Sopel works is more important to me than having proper type annotations.

So yeah, it's a first step, and this PR is mostly to put the tools in place and see if we can move forward with it. I hope none of the new dependency will be a problem.

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make quality and make test)
• I have tested the functionality of the things this change touches

[Exirel]

Fixed a conflict. Still no review, do you hate type hinting? 😄

[dgw]

Still no review, do you hate type hinting? 😄

Other things keep distracting me. Refactoring code to do the same thing isn't sexy. 😛

It's safe to say I have some questions, and some opinions. Surely you expected that and it's why you wanted a review. So here you go: a start to the process. I can't promise when I'll be able to look at this again, but then neither can you, so it works out. 😎

[Exirel]

Instead of replying to all your comments, I should have read them all, then write this long reply in one go. Sorry for the noises!

There are numerous comments that require a deeper explanation. The first thing to know: I wrote that 3 months ago, and I don't remember everything. Also, some things have changed since then (Sphinx got a new release in November, and I just saw that mypy got a release yesterday). Mypy and type-hint is moving fast in the Python community. As an example of that, the first time I tried to type-hint Sopel and use mypy (earlier this year, before September), SQLAlchemy didn't provide a stub library (or it wasn't working properly).

In any case, I'm not 100% confident in my own work. What I'm sure about, however, is my goals:

• to provide a setup to use mypy (install mypy, dealing with dependencies and stub libraries)
• to ensure that running mypy doesn't raise any warning, so it can be put into make qa in the near future
• to build upon this PR to properly implement type hint and static typing, which requires light to heavy refactoring
• to follow the Zen of Python: "practicality beats purity", as not everything has to be perfect in one go

Now, to the details.

Weak type-hint and the usage of Any

Any can be used in two cases: to represent an actual "anything will do" data, or to "pass" the static type checker. In this PR, this is clearly the second case, and yes, it is a weak typing.

Various reasons for that:

• Working incrementally, I made sure to prevent mypy warnings. Every Any is an implicit TODO: fix later
• To add a type-hint, you need the type to be accessible, i.e. either defined in the same module before using it, or by an import, which means that you must prevent cyclic import. In some case, it wasn't possible to use something else than Any for the fear of an heavy refactoring I didn't want to dive into in that PR (see previous point: working incrementally)
• Practicallity beast purity: sometimes, you gotta do what you gotta do to make it work. I'm not saying Any is good. It isn't. But right now, I don't want to write yet another PR the size of the plugin refactoring, or the bot refactoring.
• Not providing any type hint and using Any is technically the same thing, except that Any helps me remove a mypy warning that I cannot fix without an heavy refactoring, code change, and possibly breaking change.

Not all arguments

I'm sorry, I do have a reason for that, but it's a very weak one I think: mypy didn't complain about them, so I didn't spend the time to deal with all of them. I know, weak. I still think it's better to work by incremental step, to add type hint when we can where we can, one PR at a time.

Ignore typing???

I see you have been very warry of # type: ignore and I can see why. To be honest, I don't know if there is a better way to tell mypy to ignore type check for libraries that don't provide it and/or don't have type stubs.

The other option would be to impliment these stubs ourselves but... huh... I don't think it's a good idea. Especially given that I tried to use that only for Sopel's built-in plugins, and not its internal machinery.

From my perspective, the # type: ignore is explicit enough and touch only one import at a time.

Why the Seemingly unrelated changes?

That's a tough one, especially because you approached this PR with "this is only type hint" in mind, and I don't think this is the right approach. However, I'm not sure how to explain that to convince you. I'll try nonetheless, I beg your indulgence.

So, type hint, huh? What a big deal. It's about adding type annotation here and there and voilà, we are done here, right?

This is something I could have said before working with type hint on Sopel. And I would have been wrong (and I have been). In various cases, I realized that I needed an heavy refactoring to properly use type annotation (to prevent circular import for example) or I needed to type-hint a whole module before I could fix one little warning, which means figuring out how to solve a new warning, and so on. Before you knew it, I was deep into 4 or 5 level deep of refactoring, and I just said: stop.

So, whenever I had a mypy warning, I tried to find the least impactful change. Sometimes, it's type: ignore. Sometimes it's "upgrade SQLAlchemy and install its mypy stub". Sometimes, it's "OK let's remove that, or change that code so it does the same thing but differently". I didn't just change code randomly, I always did so to fix a warning.

The thing I come to understand (and by that I mean: not just in theory, but in practice) is that you have to change your code to use static typing. It is necessary and it is expected. You cannot just slap type hint here and there and expect it to work. You have to follow type theory, you have to understand static typing, you have to organize your code in a way that properly exposes the interface you want to use and properly defines the dependencies between the different parts of your code.

I knew all of that. It's nothing new. I just "discovered" what it actually means when working with a codebase like Sopel.

Yes. Expect novel-like PR in the future, there will be heavy refactoring. Just not in this PR.

What could be better?

• Argument & return type-hint: this PR is the foundation to use mypy. There is no more mypy warnings, and that's all I cared about here. So yeah, go for it once this is merged! Note that since I published this PR, it is totally possible that new mypy warning appeared, and that's why I'd like to review this PR in a timely manner (not to be confused with an emergency: this is clearly not that)
• Docstring: oh yeah, that's a big one. I think I'll revisit them in this PR.
• Protocol: I'd love to use that feature. I know there is a backport for Python 3.6 and 3.7, and I would like to dive into that next. For instance, it would be used to replace all that Any in sopel.plugin decorators, and probably into sopel.plugins and in sopel.bot as well to register a Plugin callable. This would be perfect for a future PR
• Enforce static typing in make qa: that's a move I'm not bold enough to make in a PR. I don't think Sopel is ready to enforce that rule, I don't think it's a good practice to force that change onto other contributors with a PR that keeps using Any. I think we need to build upon the foundation provided with that PR, but without adding to much burden
• # type: ignore: not that I care that much, but it would be nice to have stubs for these libraries.

And with that... I think I've replied to most of your comment. I'll try, as usual, to fix as many as possible, yet I'd like to come to an agreement on what is acceptable, and what isn't. If you think something must be fixed, it is my job to either prove you right by trying to fix it, or to prove you wrong by failing. 😜

[dgw]

I dropped specific responses into the relevant open conversations, and marked some others as resolved because they were answered by another reply.

Out of everything you said in the big omni-reply comment, there's nothing for me to really disagree on. You're absolutely right that we shouldn't enforce any of this in make qa yet, and that there are several logical next steps from here. Not enforcing the checks means we can indeed take it one thing at a time and add annotations gradually.

Assuming GitHub behaves despite my present abuse of the "review" mechanism to collect my replies and send them all at once, only stuff I still want to see changed or talk about more is still open. I'll go through the conversation list again after I submit to make sure resolved stuff stayed resolved.

[dgw]

There's a small merge conflict with #2179 now, but if you don't have any squashing/reordering to do I'm happy to just merge on the CLI and handle it, to save you the time.

[Exirel]

There's a small merge conflict

Fixed. Nothing squashed, I kept everything as it is.