plugins: turn mux-subtitles into a global argument #3324

bastimeyer · 2020-11-08T05:22:30Z

Split into two commits. See the commit messages for the descriptions.

Since the argparse docs extension had to be refactored, here's a demonstration of the overall CLI docs changes, to make sure that nothing else changed:

diff <(curl -s https://streamlink.github.io/latest/cli.html) <(cat ./docs/_build/html/cli.html)

10c10
<   <title>Command-Line Interface &mdash; Streamlink 1.7.0+52.gf0b35b6 documentation</title>
---
>   <title>Command-Line Interface &mdash; Streamlink 1.7.0+54.g49ce436 documentation</title>
31c31
<     <link rel="top" title="Streamlink 1.7.0+52.gf0b35b6 documentation" href="index.html"/>
---
>     <link rel="top" title="Streamlink 1.7.0+54.g49ce436 documentation" href="index.html"/>
56c56
<   <span class="wy-side-nav-versions-current"><i class="fa fa-book"></i>1.7.0+52.gf0b35b6</span>
---
>   <span class="wy-side-nav-versions-current"><i class="fa fa-book"></i>1.7.0+54.g49ce436</span>
1382a1383,1390
> <dl class="option">
> <dt id="cmdoption-mux-subtitles">
> <code class="descname">--mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-mux-subtitles" title="Permalink to this definition">¶</a></dt>
> <dd><p>Automatically mux available subtitles into the output stream.</p>
> <p>Needs to be supported by the used plugin.</p>
> <p><strong>Supported plugins:</strong> funimationnow, pluzz, rtve, svtplay, vimeo</p>
> </dd></dl>
> 
1610,1616d1617
< <dt id="cmdoption-funimation-mux-subtitles">
< <code class="descname">--funimation-mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-funimation-mux-subtitles" title="Permalink to this definition">¶</a></dt>
< <dd><p>Enable automatically including available subtitles in to the output
< stream.</p>
< </dd></dl>
< 
< <dl class="option">
1688,1699d1688
< <dt id="cmdoption-pluzz-mux-subtitles">
< <code class="descname">--pluzz-mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-pluzz-mux-subtitles" title="Permalink to this definition">¶</a></dt>
< <dd><p>Automatically mux available subtitles in to the output stream.</p>
< </dd></dl>
< 
< <dl class="option">
< <dt id="cmdoption-rtve-mux-subtitles">
< <code class="descname">--rtve-mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-rtve-mux-subtitles" title="Permalink to this definition">¶</a></dt>
< <dd><p>Automatically mux available subtitles in to the output stream.</p>
< </dd></dl>
< 
< <dl class="option">
1742,1747d1730
< <dt id="cmdoption-svtplay-mux-subtitles">
< <code class="descname">--svtplay-mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-svtplay-mux-subtitles" title="Permalink to this definition">¶</a></dt>
< <dd><p>Automatically mux available subtitles in to the output stream.</p>
< </dd></dl>
< 
< <dl class="option">
1813,1818d1795
< <dt id="cmdoption-vimeo-mux-subtitles">
< <code class="descname">--vimeo-mux-subtitles</code><code class="descclassname"></code><a class="headerlink" href="#cmdoption-vimeo-mux-subtitles" title="Permalink to this definition">¶</a></dt>
< <dd><p>Automatically mux available subtitles in to the output stream.</p>
< </dd></dl>
< 
< <dl class="option">
1925c1902
<             VERSION:'1.7.0+52.gf0b35b6',
---
>             VERSION:'1.7.0+54.g49ce436',

back-to · 2020-11-08T08:05:42Z

might be useful if you also add this plugin option to the log output

plugin.global_arguments

streamlink/src/streamlink_cli/main.py

Lines 557 to 564 in 49ce436

    
           plugin_args = [] 
        
           for parg in plugin.arguments: 
        
               value = plugin.get_option(parg.dest) 
        
               if value: 
        
                   plugin_args.append((parg, value)) 
        
           if plugin_args: 
        
               log.debug("Plugin specific arguments:")

maybe not important for mux-subtitles,
but if we decide to add purge-credentials username password in the future as global args it would be better with a log output.

bastimeyer · 2020-11-08T23:23:01Z

That would require refactoring the Argument class or adding a separate class for the global arguments, because the arguments of the global arguments list are just references and will be logged differently (incorrectly). And arguments with sensitive data make this even more complicated, because the global argument which is being referenced needs to be looked up.

Regarding the other arguments which you've mentioned, I'm not sure if making them global is a good idea, because they depend on the input URL what's being done, and this can be confusing or lead to user issues when used incorrectly, especially purge-credentials. Global username/email/password/token/etc arguments could work, but since there's no unique scheme/pattern, that would require having a lot of global arguments and the list of supported plugins would be confusing. The mux-subtitles argument works in a global context because it's an independent thing.

src/streamlink/plugins/vimeo.py

bastimeyer · 2020-11-09T18:21:01Z

Added the is_global parameter to the Argument class and refactored the streamlink_cli.main methods. The global plugin arguments will now correctly be debug-logged (what @back-to mentioned), just like the regular plugin arguments. Documentation-wise, nothing has changed from the previous implementation.

Just noticed some changes I've missed which should have been reverted. Will force-push again in a minute...

- Add `is_global` parameter to `streamlink.options.Argument` Global plugin arguments reference Streamlink's global arguments by name and cannot receive any other parameters, like `required`, etc. - In `streamlink_cli.main.setup_plugin_args`, find the global argument references of each plugin, set the available default values and add the plugin to the argument's `plugins` list, which will be read by the docs generator - In `streamlink_cli.main.setup_plugin_options`, set the provided global argument values on the plugin's `options` dictionary - Refactor docs/ext_argparse - Don't override argparse.ArgumentParser when importing the parser - Add list of supported plugins to the parameter description if the `plugins` list is set on the argparse action object - Add/improve/fix tests

This removes the old plugin-specific arguments: - `--funimation-mux-subtitles` - `--pluzz-mux-subtitles` - `--rtve-mux-subtitles` - `--svtplay-mux-subtitles` - `--vimeo-mux-subtitles` and replaces it with a single global `--mux-subtitles` argument. Also adds `mux-subtitles` to the Session options.

beardypig · 2020-11-10T09:17:00Z

Looks good, but I'd like to add somethings to the tests. Lemme see if it's feasible....

beardypig · 2020-11-10T09:25:43Z

As an additional note, for this to really make sense I think we need to move the argument definition from the CLI module in to the main streamlink module otherwise the global arguments are coupled to the CLI.

beardypig · 2020-11-10T09:45:40Z

The test I added shows the strong coupling between the CLI and the main module.

beardypig · 2020-11-10T09:47:16Z

docs/ext_argparse.py

+            if hasattr(action, "plugins") and len(action.plugins) > 0:
+                yield f"    **Supported plugins:** {', '.join(action.plugins)}"
+                yield ""


nice touch!

beardypig

Looks good to me. @bastimeyer should take a look at the meta-test I added though.

bastimeyer · 2020-11-10T11:19:04Z

Test looks good... Theoretically this could even be extended a bit by checking that no other argument parameters are set, as the global arguments are not supposed to receive any parameters other than the name, but I don't think it's necessary.

for this to really make sense I think we need to move the argument definition from the CLI module in to the main streamlink module otherwise the global arguments are coupled to the CLI.

That was already the case previously with the regular plugin arguments, because they are reading their values from streamlink_cli.main.args as well. Strictly speaking, there is no dependency on streamlink_cli, as streamlink_cli itself sets the values on the plugin's options dict, and without it, the dict is just empty, but that doesn't affect the plugin logic.

Why do we even have a separate args variable, when the session.options dict exists? Isn't that supposed to contain all argument values? Right now, it's just an additional dict with hand-picked arguments and a little bit of value mapping/customization (which btw isn't particularly efficient).

beardypig · 2020-11-10T14:32:41Z

I mean that the definition of the "global arguments" should be moved to the streamlink module, probably using a class similar to PluginArgument to define them. The important aspect here is the definition of the arguments. There is no definition of what mux-subtitles is outside of the streamlink_cli module, which is why the new test I wrote has to import streamlink_cli.argparser.build_parser.

streamlink_cli.main.args is the direct output from the argparse parser, there still needs to be a separation between the CLI and Streamlink arguments, all the player options, file writing options, etc. are interface specific but all the HLS options, HTTP options, etc. could have their definitions moved to Streamlink. I am imagining the Streamlink/Pluigin arguments just have to be defined in one place, this would make a new front-end implementation a bit easier.

bastimeyer · 2020-11-11T04:54:22Z

there still needs to be a separation between the CLI and Streamlink arguments, all the player options, file writing options

True, I didn't think about that while writing my previous comment.

If you want to move all "global arguments" to streamlink, then this would include every argument that's not strictly CLI-related, so basically everything that's in the Session options, and you'd have to feed those arguments to streamlink_cli in a similar fashion as the plugin arguments.

That's not an issue of this PR though.

Btw, decoupling plugin arguments from their plugin modules is another issue and would probably an annoyance later on if we change/improve the plugin import system.

beardypig · 2020-11-11T18:17:12Z

That is what I was thinking. Perhaps I will solidify my ideas in to an issue/PR to be worked on/discussed.

bastimeyer · 2020-11-14T07:46:20Z

Any further concerns? I think this should be good now. All other things mentioned here are unrelated to the specific changes here.

- Add `is_global` parameter to `streamlink.options.Argument` Global plugin arguments reference Streamlink's global arguments by name and cannot receive any other parameters, like `required`, etc. - In `streamlink_cli.main.setup_plugin_args`, find the global argument references of each plugin, set the available default values and add the plugin to the argument's `plugins` list, which will be read by the docs generator - In `streamlink_cli.main.setup_plugin_options`, set the provided global argument values on the plugin's `options` dictionary - Refactor docs/ext_argparse - Don't override argparse.ArgumentParser when importing the parser - Add list of supported plugins to the parameter description if the `plugins` list is set on the argparse action object - Add/improve/fix tests streamlink#3324

bastimeyer added the enhancement label Nov 8, 2020

bastimeyer requested a review from beardypig November 8, 2020 05:23

beardypig reviewed Nov 9, 2020

View reviewed changes

src/streamlink/plugins/vimeo.py Outdated Show resolved Hide resolved

bastimeyer force-pushed the mux-subtitles branch from 49ce436 to 3c2554e Compare November 9, 2020 18:15

bastimeyer force-pushed the mux-subtitles branch from 3c2554e to dc319e0 Compare November 9, 2020 18:22

bastimeyer added 2 commits November 10, 2020 06:58

bastimeyer force-pushed the mux-subtitles branch from dc319e0 to b3e0c67 Compare November 10, 2020 05:59

tests: validate all plugins' global arguments

664224e

beardypig reviewed Nov 10, 2020

View reviewed changes

beardypig approved these changes Nov 10, 2020

View reviewed changes

bastimeyer mentioned this pull request Nov 10, 2020

The road towards 2.0.0 #3235

Closed

25 tasks

bastimeyer mentioned this pull request Nov 13, 2020

docs: switch theme to furo #3335

Merged

bastimeyer requested a review from back-to November 14, 2020 07:46

back-to merged commit d0b6598 into streamlink:master Nov 14, 2020

bastimeyer deleted the mux-subtitles branch January 19, 2021 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

plugins: turn mux-subtitles into a global argument #3324

plugins: turn mux-subtitles into a global argument #3324

Uh oh!

bastimeyer commented Nov 8, 2020

Uh oh!

back-to commented Nov 8, 2020

Uh oh!

bastimeyer commented Nov 8, 2020

Uh oh!

Uh oh!

bastimeyer commented Nov 9, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig Nov 10, 2020

Uh oh!

beardypig left a comment

Uh oh!

bastimeyer commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

bastimeyer commented Nov 11, 2020

Uh oh!

beardypig commented Nov 11, 2020

Uh oh!

bastimeyer commented Nov 14, 2020

Uh oh!

Uh oh!

Uh oh!

plugins: turn mux-subtitles into a global argument #3324

plugins: turn mux-subtitles into a global argument #3324

Uh oh!

Conversation

bastimeyer commented Nov 8, 2020

Uh oh!

back-to commented Nov 8, 2020

Uh oh!

bastimeyer commented Nov 8, 2020

Uh oh!

Uh oh!

bastimeyer commented Nov 9, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

beardypig Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

beardypig left a comment

Choose a reason for hiding this comment

Uh oh!

bastimeyer commented Nov 10, 2020

Uh oh!

beardypig commented Nov 10, 2020

Uh oh!

bastimeyer commented Nov 11, 2020

Uh oh!

beardypig commented Nov 11, 2020

Uh oh!

bastimeyer commented Nov 14, 2020

Uh oh!

Uh oh!