options: refactor Argument/pluginargument #5715
Conversation
19ab00d to
a1f8ec9
Compare
I don't think this potential edge case should be high on the list of considerations. The restriction trade off for the clarity we'll gain is worth it. |
|
Going to update and rebase this PR after #5778 got merged.
I will remove this again, because it's not important. The This way, plugins can have custom argument types and the script which parses the plugin matchers and plugin arguments can properly serialize the plugin arguments data into JSON. |
- Explicitly define keywords passed to `ArgumentParser.add_argument()` so they can be properly type-checked - Update `Argument.options` accordingly and add tests - Fix `Argument.requires` value - Update docstrings
a1f8ec9 to
5b243a5
Compare
This explicitly adds all keywords which are passed to
argparse.ArgumentParser.add_argument()to thepluginargumentdecorator (Argumentclass).It also turns all options except fornameinto mandatory keywords (which is technically a breaking change, but I don't think anyone with common sense has defined custom plugin arguments using positional args).The intention here is simplifying the parsing of the plugin module AST when building a plugins JSON file with all matchers and arguments (#4741), so Streamlink doesn't have to load all plugins at once. This is what I've been working on again in the past couple of days. It also improves type checking of all plugin arguments.
Opening this as a draft for now, because I don't want these changes to be included in the next release yet.
Unrelated to this PR, there's also one issue left with these changes which I will have to think about, namely the argument's
typeoption.The
typeoption is currently defined asCallable[[Any], Any] | None, but this makes it impossible to serialize when building the plugins JSON data.The data serializer needs to implement an import path lookup, so the
typeoption can reference the actual callable again, e.g.streamlink.utils.args.keyvalueorstreamlink.utils.times.hours_minutes_seconds. These are currently the two functions being used by Streamlink's plugins, but other callables like builtin ones need to be supported as well, likeintfor example. It will however restrict any custom functions defined in plugin modules, should this ever be required. Third party plugins won't be affected by the pluginargument serialization/deserialization, because those have to be loaded all at once beforehand anyway.