Skip to content

plugin: add session typing information #4802

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

Merged

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Sep 4, 2022

  • Add typing information to Plugin.session
  • Implement http_session stub file due to the HTTPSession subclass
    of requests.Session which adds additional keywords to the
    request() method, including all other HTTP-verb methods
  • Add flake8-pyi and typing_extensions to dev-requirements.txt
    (typing_extensions is not a runtime dependency)

With the removal of the Plugin.bind() classmethod in #4768, now that the Streamlink session instance gets set on each Plugin instance, typing information can finally be added, as the session attribute doesn't have to be defined as None anymore initially.

Adding typing informations finally allows plugin implementors to see all the stuff that's defined on the Session, including the HTTPSession via self.session.http. However, since Streamlink subclasses requests's Session and adds additional keywords to the request() method and thus also to all HTTP-verb methods like get(), post(), etc., this introduces typing issues. The most common problem by far is of course the addition of the schema keyword, which also changes the return type of the various methods from Response to Any.

I originally tried to avoid adding a stub file and directly adding the missing HTTP-verb methods to the HTTPSession subclass and adding typing information there, but that got very messy. The repetitive typing information therefore needs to be defined in a stub file. See PEP561.

Unfortunately though, it doesn't seem to be possible to import already defined type aliases from the types-requests package, so I had to copy all required definitions for the other args+keywords provided by requests.


The typing_extensions dependency had to be added because mypy is configured for python 3.7, and TypeAlias wasn't part of the typing module yet in 3.7. Otherwise a error: Module "typing" has no attribute "TypeAlias" would be raised.

The new Y037 linting error also has to be ignored because of an apparent mypy bug (error: Type application has too many types (1 expected)) when union types like a | b | c are defined instead of Union[a,b,c]. This is all a bit weird, because the copied type alias defintions are using both styles. Maybe it's because of the re-defintions... The union types syntax in stub files is valid on Python 3.7, btw:
https://typing.readthedocs.io/en/latest/source/stubs.html#syntax


Opening this as a draft for now.

@bastimeyer bastimeyer force-pushed the plugin/session-typing-information branch from 64ba278 to 7ae0796 Compare September 5, 2022 18:50
@bastimeyer bastimeyer changed the title plugin: add Plugin.session typing information plugin: add session typing information Sep 5, 2022
@bastimeyer bastimeyer force-pushed the plugin/session-typing-information branch from 7ae0796 to 88756ff Compare September 5, 2022 19:00
- Add typing information to `Plugin.session`
- Implement `http_session` stub file due to the `HTTPSession` subclass
  of `requests.Session` which adds additional keywords to the
  `request()` method, including all other HTTP-verb methods
- Add `flake8-pyi` and `typing_extensions` to dev-requirements.txt
  (`typing_extensions` is not a runtime dependency)
@bastimeyer bastimeyer force-pushed the plugin/session-typing-information branch from 88756ff to 289434b Compare September 9, 2022 23:09
@bastimeyer bastimeyer marked this pull request as ready for review September 9, 2022 23:09
@bastimeyer
Copy link
Member Author

I've changed the return type of the request() and HTTP-verb methods to Any, because Response | Any doesn't work well. My idea was to have the default return type included in addition to Any, so that code that actually calls methods or reads attributes on Response objects can have typing information, but apparently this results in the Response | Response typing information in PyCharm, which causes warnings for all kinds of stuff. No idea why, but it's annoying and can be fixed/improved later.

Once this PR is merged, I will open another one with typing information added to the various stream implementations. Adding this here unnecessarily bloats up the diff.

@gravyboat
Copy link
Member

Thanks @bastimeyer. Bummer about the Response | Any types since it would be really nice, seems weird that it would infer the Any as Response but as you mentioned something along the line must be causing weird inference.

@gravyboat gravyboat merged commit 95df07c into streamlink:master Sep 10, 2022
@bastimeyer bastimeyer deleted the plugin/session-typing-information branch September 10, 2022 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants