options: make Options inherit from dict #6311
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of using an internal `options` dict attribute, make `Options` inherit from `dict[str, Any]` directly, so both `Options` and `dict`s can be merged in `update()`. Update tests and also explicitly override the typing annotations of `get()` (no default value) and `update()` (only single mapping arg).
5eecceb to
61f1502
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes the
optionsarguments of both theStreamlinksession class and thePluginclass more consistent. The former currently doesn't allow passing anOptionsobject as theoptionsargument, while the latter doesn't allowdicts and always requires anOptionsobject.The reason for these inconsistencies is that the
Streamlinksession defines a set of default values and key-value mappings, so passing anOptionsobject didn't make sense when it was implemented. ThePluginoptions argument was implemented when thePlugin.bind()nonsense was removed andPlugin.optionswere turned from class attributes to instance attributes (5.0.0 - #5033), so regulardicts were never considered as plugin options arguments.A side-effect of these changes however is that plugin options now don't have default values anymore, because the argument is always merged into an emptyOptionsobject. I'm not fully sure if this is actually relevant here in the plugin API. I'll have to think about it, because I don't want to break any downstream stuff. Opening this as a draft for now because of that.Custom plugin options are now set as default values on the plugin's
optionsobject, copied from the passeddictorOptionsobject.