Skip to content

session: refactor option getter+setter methods #5076

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
merged 3 commits into from
Jan 8, 2023

Conversation

bastimeyer
Copy link
Member

This replaces the logic of the Streamlink.{get,set}_option() methods with a new StreamlinkOptions subclass which uses mapped option getters+setters that apply the custom option logic instead.

The motivation for this is that this improves the runtime of said methods, as option mappers are looked up in Python dictionaries in constant time, so option names don't have to be checked one after another. The old implementation is also really really bad code-wise and not extensible.

In order to do this, the Options class had to be refactored a bit and extended:

  1. Previously, the internally stored option keys were normalized with argument names of the argparse.Namespace in mind, so dashes were replaced with underscores. Since Streamlink's session options are defined using dashes, I changed the normalization the other way around, to make it consistent. For various plugin options, this doesn't have any effect (e.g. see various "purge_credentials" options). This is all properly tested and no further changes are required.
  2. The second commit adds support for mapped getter and setter functions, so that keys can be looked up in O(1). This is why the change of key-name normalization was necessary, to be able to define getters and setters in a normalized way that's consistent with the default options of the Streamlink session.

The third commit then finally cleans up the Streamlink session and moves the special option logic into the newly added StreamlinkOptions subclass, and it also adds new deprecation messages (see commit message). The individual getters and setters were not modified except for

  • http-disable-dh, which now has a proper getter
  • https-proxy, which now shows a deprecation message on its getter (still returns the "https" proxy, even though both "http" and "https" get set by the setter, because the proxies can also be defined by requests via env vars)

I only added new tests for the mapped options implementation and http{,s}-proxy getter. Some of the Streamlink session options are not actually tested, e.g. some HTTP session attributes, but all the getters and setters should be covered, and adding tests for simple setattr/getattr calls isn't too important. Current code coverage (can't permalink with commit-id, so this will change):
https://app.codecov.io/gh/streamlink/streamlink/blob/master/src/streamlink/session.py


If this PR gets merged, then this will cause merge conflicts with two of my other open (draft) PRs and those will need a quick update:

- Store key names with dashes instead of underscore characters.
  This makes it consistent with the default Streamlink session options.
- Add typing information
Replace the logic of the `Streamlink.{get,set}_option()` methods with
a new `StreamlinkOptions` subclass which uses mapped option
getters+setters that apply the custom option logic instead.

This improves the runtime of said methods, as option mappers are
looked up in Python dictionaries in constant time, so option names
don't have to be checked one after another.

Also add deprecation messages when
- getting the deprecated `https-proxy` option
- setting the deprecated `{hls,dash}-segment-attempts` options
- setting the deprecated `{hls,dash}-segment-threads` options
- setting the deprecated `{hls,dash}-segment-timeout` options
- setting the deprecated `{hls,dash,http-stream}-timeout` options
Copy link
Member

@gravyboat gravyboat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @bastimeyer, the code is much easier to parse as you go through it.

@gravyboat gravyboat merged commit 5e3719f into streamlink:master Jan 8, 2023
@bastimeyer bastimeyer deleted the session/refactor-options branch January 8, 2023 02:06
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