-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typehints #398
base: main
Are you sure you want to change the base?
Add typehints #398
Conversation
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. |
Okay, I'll do it when I'm on the PC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- Find bugs or help our IDE provide relevant feedback in our own codebase
- 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 !).
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... |
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 For example:
In other words, until we add a FWIW, mypy apparently also needs this
That's a good argument in favor of B I believe. |
No description provided.