-
-
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
|
…ments Original PR #5033 by bastimeyer Original: streamlink/streamlink#5033
…move global plugin arguments Merged from original PR #5033 Original: streamlink/streamlink#5033
These are breaking changes of the
Streamlinksession andPluginAPIs.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.optionsto plugin instancesAs mentioned in #5011, turning
Plugin.optionsfrom 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.0release. While5.0.0removedPlugin.bind(), the breaking changes were actually in theStreamlinksession class wherePlugin.bind()was called internally, and compatibility wrappers were added to thePluginconstructor, with deprecation messages for old-style parameters. This here is another set of breaking changes on theStreamlinksession class andPluginclass, 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
Pluginclass previously shared a commonOptionsinstance as thePlugin.optionsclass 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 newOptionsinstance.This design is bad for several reasons. First, this makes
streamlink_clia "soft" dependency of thePluginandStreamlinkclasses, as all plugin options would otherwise be shared between all plugins on the sameOptionsinstance. 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.optionsclass attribute when initializing multiple plugins, which is bad.Now, after these changes:
Pluginclass can receive an optionalOptionsinstance as a parameter on its constructor, which streamlink_cli uses for setting up individual plugin options. Having it in thePluginconstructor 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 thekeywordsparameter toHLSStream.parse_variant_playlist()to be able to pass keywords to the{,Muxed}HLSStreamclasses. In case of the Twitch plugin, these keywords aredisable_adsandlow_latencywhich are read from the plugin options instead ofsession.get_plugin_option().2. Removal of
Argument.is_globalI'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_globalwas 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").