-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Move plugin options to plugin instances and remove global plugin arguments #5033
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
Conversation
335e846
to
e5fc001
Compare
e51fe0f
to
46c28f7
Compare
Rebased to master and fixed merge conflict |
4ef1049
to
ae946fd
Compare
Rebased to master and fixed merge conflict again. No other changes. I'm not happy about the |
1207bcf
to
90ad58c
Compare
Rebased to master and resolved new merge conflicts:
|
Plugin options are currently shared between all plugin instances via the `Plugin.options` class attribute, unless it gets overridden by Streamlink's CLI via `setup_plugin_args()` after reading the plugin arguments, updating the argument parser and reading the default values. This allows for accidental pollution of the plugin options when not overriding the `options` class attribute with a new `Options` instance. Since this is only done by `streamlink_cli`, this is an issue in tests and third party python projects using the Streamlink API. - Remove `Plugin.options` class attribute and store an `Options` instance on the `Plugin` instance instead, and allow passing an already initialized options instance to the `Plugin` constructor - Turn `Plugin.{g,s}et_option()` from class methods to regular methods - Update CLI and initialize the resolved plugin with an options instance that gets built by `setup_plugin_options()`, with default values read from the CLI arguments - Remove `Streamlink.{g,s}et_plugin_option()` and fix usages: - Session and Options tests - Twitch plugin, TwitchAPI, TwitchHLSStream, and their tests - Add `keywords` mapping to `HLSStream.parse_variant_playlist`, to be able to pass custom keywords to the `{,Muxed}HLSStream` - Move and rewrite CLI plugin args+options integration tests
90ad58c
to
b91e9a4
Compare
Rebased to master again and fixed merge conflicts. No other changes. |
Please remove set_plugin_option from docs: https://streamlink.github.io/api.html and put there explanation and example of how to set plugin options for now |
Have a look at the latest docs.
streamlink/src/streamlink/plugin/plugin.py Line 273 in 628e8ab
|
These are breaking changes of the
Streamlink
session andPlugin
APIs.Opening this as a draft for now, even though it's pretty much final, because I don't think that this has to be merged now. It simply fixes an old design flaw and is not too important and should be included with other breaking changes in the future in one go.
Two separate changes:
1. Moving
Plugin.options
to plugin instancesAs mentioned in #5011, turning
Plugin.options
from a plugin class attribute into a plugin instance attribute is related to the removal of thePlugin.bind()
class method in #4744 / #4768, and ideally should've been part of the5.0.0
release. While5.0.0
removedPlugin.bind()
, the breaking changes were actually in theStreamlink
session class wherePlugin.bind()
was called internally, and compatibility wrappers were added to thePlugin
constructor, with deprecation messages for old-style parameters. This here is another set of breaking changes on theStreamlink
session class andPlugin
class, but without any deprecations.Just to recap what I've said in #5011 and what I've included in the commit messages of this PR, the
Plugin
class previously shared a commonOptions
instance as thePlugin.options
class attribute. This class attribute was then overridden bystreamlink_cli.main.setup_plugin_args()
where plugin arguments were processed and added to the argparser while reading default values of the plugin arguments that were added to a newOptions
instance.This design is bad for several reasons. First, this makes
streamlink_cli
a "soft" dependency of thePlugin
andStreamlink
classes, as all plugin options would otherwise be shared between all plugins on the sameOptions
instance. Second, multiple instances of the same plugin always have the same set of options set (not really important, but still bad). And third, tests and third party projects not using streamlink_cli need to manually override/clear thePlugin.options
class attribute when initializing multiple plugins, which is bad.Now, after these changes:
Plugin
class can receive an optionalOptions
instance as a parameter on its constructor, which streamlink_cli uses for setting up individual plugin options. Having it in thePlugin
constructor is important, because plugin subclasses need to be able to read plugin options immediately (see Twitch and TwitchAPI), and not after initialization.Streamlink.get_plugin_option()
andStreamlink.set_plugin_option()
have been removed. These methods were useful for sharing state between different parts of Streamlink, e.g. plugins and streams (see Twitch and TwitchHLSStream), but this can be done in different ways. The PR therefore also adds thekeywords
parameter toHLSStream.parse_variant_playlist()
to be able to pass keywords to the{,Muxed}HLSStream
classes. In case of the Twitch plugin, these keywords aredisable_ads
andlow_latency
which are read from the plugin options instead ofsession.get_plugin_option()
.2. Removal of
Argument.is_global
I've already had this on my radar for a while. Since the changed CLI methods already had to deal with global plugin arguments, I decided to include the removal of global plugin arguments in this PR.
The reason for the removal of global plugin arguments is simple: it adds unnecessary complexity for absolutely no gain. The value which a global plugin argument stores is already stored in the session options and should be read from there.
is_global
was added when we removed individual plugin arguments for muxing subtitles, but no other global plugin arguments were ever added. The only benefit it had was being able to include a list of plugins in the docs which were making use of--mux-subtitles
, as the global plugin arguments allowed for parsing this data. This additional data in the docs is now gone as well, but only affects four plugins. If we want to have a list of certain features a plugin uses, then this can be done by extending the regular plugin metadata.What plugin implementors have to do to fix this is very simple, remove any global plugin arguments and change the option lookup, e.g.
self.get_option("mux_subtitles")
toself.session.get_option("mux-subtitles")
.