Add typehints

[gophra]

No description provided.

[moy]

I had a quick look and it looks good to me (not a real review yet). Note that in this project, we try to keep a clean history hence every commit should have a nice commit message. If possible, please squash your commits before we merge, otherwise we'll do that while merging.

[gophra]

Okay, I'll do it when I'm on the PC

[hemberger]

LGTM, thanks for your contribution!

[moy]

The PR is not really wrong per-se, but adds new warnings from mypy and pyright.

Adding typehints can be done with two goals in mind:

1. Find bugs or help our IDE provide relevant feedback in our own codebase
2. Find bugs or help IDEs provide relevant feedback for users of MechanicalSoup

Having more warnings with than without the PR is counter-productive wrt objective 1., but still valid for 2.

An example of new warning:

    def add_soup(response: requests.Response, soup_config):
            ...
            response.soup = bs4.BeautifulSoup(
                ...

Now that the typechecker knows that response's type, it knows that the .soup attribute doesn't exist, and complains. See e.g. https://stackoverflow.com/questions/61213745/typechecking-dynamically-added-attributes for a solution.

Unfortunately, I believe users of MechanicalSoup will have the same issue if they access the .soup attribute.

I think we should at least discuss this in the commit message.

Then, I see two ways to proceed:
A. decide that this PR in its current state is counter-productive and wait for (or write ;-) ) a better PR.
B. consider this PR as a good first step, and merge it (with an improved commit message).

I'd vote for B. (and in any case, A. should not be considered as a discouragement !).

[hemberger]

I was afraid that might be the case. We probably should have made the response + soup object a new class, rather than hijack the response object. I guess that's one of the inherent dangers with Python's duck typing framework -- it can be challenging to shoehorn traditional typing in after the fact (though I think it's a very worthy goal!).

Please correct me if I'm wrong, but I don't think mypy will actually use any of our typehints (when MechanicalSoup is an imported library) until we provide stubs. In that case, the presence of the typehints shouldn't introduce any warnings into user projects yet. So I'm fine with merging as-is, but we may want to first make sure this problem can even be solved...

[moy]

Please correct me if I'm wrong, but I don't think mypy will actually use any of our typehints (when MechanicalSoup is an imported library) until we provide stubs.

That seems true with mypy. Pyright (more recent, and in my experience better) has a slightly different behavior: it will use typing annotations if mechanicalsoup is imported from a subdirectory (i.e. you copied the mechanicalsoup directory in your project) or if the mechanicalsoup directory contains a py.typed file (possibly empty, it's just a marker saying "do consider type here").

For example:

$ cat test.py                                                             
import mechanicalsoup

browser = mechanicalsoup.Browser()
page = browser.get("http://example.com")
reveal_type(page)
$ pyright test.py 
No configuration file found.
No pyproject.toml file found.
stubPath /tmp/test/typings is not a valid directory.
Assuming Python platform Linux
Searching for source files
Found 1 source file
/tmp/test/test.py
  /tmp/test/test.py:5:13 - info: Type of "page" is "Unknown"
0 errors, 0 warnings, 1 info 
Completed in 0.809sec
$ touch /home/mmoy/.local/lib/python3.8/site-packages/mechanicalsoup/py.typed
$ pyright test.py                                                            
No configuration file found.
No pyproject.toml file found.
stubPath /tmp/test/typings is not a valid directory.
Assuming Python platform Linux
Searching for source files
Found 1 source file
/tmp/test/test.py
  /tmp/test/test.py:5:13 - info: Type of "page" is "Response"
0 errors, 0 warnings, 1 info 
Completed in 0.915sec

In other words, until we add a py.typed file to our install, we can safely add type hints without bothering our users in case our hints are too strict.

FWIW, mypy apparently also needs this py.typed but also needs a stub file while pyright can extract type directly inlined into the code:

$ mypy test.py
test.py:5: note: Revealed type is "Any"

That's a good argument in favor of B I believe.