From d24048bb015031fafc6fcdb3d47d44114df3c298 Mon Sep 17 00:00:00 2001 From: Jesse Bannon Date: Tue, 25 Jul 2023 19:51:50 -0700 Subject: [PATCH] [REFACTOR] No more download strategy (#669) * [REFACTOR] No more download strategy * remove more --- src/ytdl_sub/config/preset.py | 168 +----------------- src/ytdl_sub/config/preset_class_mappings.py | 79 -------- src/ytdl_sub/downloaders/url/downloader.py | 3 +- .../subscriptions/base_subscription.py | 15 +- .../subscriptions/subscription_download.py | 5 +- .../subscription_ytdl_options.py | 5 - tests/unit/config/test_config_file.py | 43 ----- 7 files changed, 12 insertions(+), 306 deletions(-) diff --git a/src/ytdl_sub/config/preset.py b/src/ytdl_sub/config/preset.py index fdedfdb9..f5ecd3ac 100644 --- a/src/ytdl_sub/config/preset.py +++ b/src/ytdl_sub/config/preset.py @@ -12,14 +12,13 @@ from mergedeep import mergedeep from ytdl_sub.config.config_validator import ConfigValidator from ytdl_sub.config.plugin import Plugin -from ytdl_sub.config.preset_class_mappings import DownloadStrategyMapping from ytdl_sub.config.preset_class_mappings import PluginMapping from ytdl_sub.config.preset_options import OptionsValidator from ytdl_sub.config.preset_options import OutputOptions from ytdl_sub.config.preset_options import Overrides from ytdl_sub.config.preset_options import TOptionsValidator from ytdl_sub.config.preset_options import YTDLOptions -from ytdl_sub.downloaders.source_plugin import SourcePlugin +from ytdl_sub.downloaders.url.validators import MultiUrlValidator from ytdl_sub.entries.entry import Entry from ytdl_sub.prebuilt_presets import PREBUILT_PRESET_NAMES from ytdl_sub.prebuilt_presets import PUBLISHED_PRESET_NAMES @@ -34,16 +33,15 @@ from ytdl_sub.validators.string_formatter_validators import StringFormatterValid from ytdl_sub.validators.validators import DictValidator from ytdl_sub.validators.validators import ListValidator from ytdl_sub.validators.validators import StringListValidator -from ytdl_sub.validators.validators import StringValidator from ytdl_sub.validators.validators import Validator from ytdl_sub.validators.validators import validation_exception PRESET_KEYS = { "preset", + "download", "output_options", "ytdl_options", "overrides", - *DownloadStrategyMapping.sources(), *PluginMapping.plugins(), } @@ -101,49 +99,6 @@ class PresetPlugins: return None -class DownloadStrategyValidator(StrictDictValidator): - """ - Ensures a download strategy exists for a source. Does not validate any more than that. - The respective Downloader's option validator will do that. - """ - - # All media sources must define a download strategy - _required_keys = {"download_strategy"} - - # Extra fields will be strict-validated using other StictDictValidators - _allow_extra_keys = True - - def __init__(self, name: str, value: Any): - super().__init__(name=name, value=value) - self.download_strategy_name = self._validate_key( - key="download_strategy", - validator=StringValidator, - ).value - - def get(self, downloader_source: str) -> Type[SourcePlugin]: - """ - Parameters - ---------- - downloader_source: - Name of the download source - - Returns - ------- - The downloader class - - Raises - ------ - ValidationException - If the download strategy is invalid - """ - try: - return DownloadStrategyMapping.get( - source=downloader_source, download_strategy=self.download_strategy_name - ) - except ValueError as value_exc: - raise self._validation_exception(error_message=value_exc) - - class _PresetShell(StrictDictValidator): # Have all present keys optional since parent presets could not have all the # required keys. They will get validated in the init after the mergedeep of dicts @@ -152,70 +107,6 @@ class _PresetShell(StrictDictValidator): class Preset(_PresetShell): - @classmethod - def _validate_download_strategy( - cls, config: ConfigValidator, name: str, value: Dict, parent_presets: StringListValidator - ) -> None: - sources: List[str] = [] - for source_name in DownloadStrategyMapping.sources(): - if source_name in value: - sources.append(source_name) - - if len(sources) > 1: - raise validation_exception( - name=name, - error_message=f"Contains the sources {', '.join(sources)} but can only have one", - ) - - # If no sources, nothing more to validate - if not sources: - return - - # Ensure all parent/child presets use the same download strategy - preset_strs = [preset.value for preset in parent_presets.list] + [name] - download_strategy: Optional[str] = None - download_strategy_preset: Optional[str] = None - for parent_preset in preset_strs: - # See if the parent has a download strategy - parent_download_strategy = ( - config.presets.dict.get(parent_preset, {}) - .get("download", {}) - .get("download_strategy") - ) - - # If the parent download strategy is None, do nothing - if parent_download_strategy is None: - continue - - # If download strategy is not set, then set it to parent download strategy - if download_strategy is None: - download_strategy = parent_download_strategy - download_strategy_preset = parent_preset - - # If it's set and does not match the parent download strategy, error - if download_strategy is not None and download_strategy != parent_download_strategy: - raise ValidationException( - f"Preset {download_strategy_preset} uses download strategy {download_strategy}" - f", but is inherited by preset {parent_preset} which uses download strategy " - f"{parent_download_strategy}." - ) - - source_name = sources[0] - source_dict = copy.deepcopy(value[source_name]) - - # Set the parent download strategy if present - if download_strategy: - source_dict["download_strategy"] = download_strategy - - downloader = DownloadStrategyValidator(name=f"{name}.{source_name}", value=source_dict).get( - downloader_source=source_name - ) - del source_dict["download_strategy"] - - downloader.plugin_options_type.partial_validate( - name=f"{name}.{source_name}", value=source_dict - ) - @classmethod def preset_partial_validate(cls, config: ConfigValidator, name: str, value: Any) -> None: """ @@ -250,9 +141,7 @@ class Preset(_PresetShell): presets=config.presets.keys, ) - cls._validate_download_strategy( - config=config, name=name, value=value, parent_presets=parent_presets - ) + cls._partial_validate_key(name, value, "download", MultiUrlValidator) cls._partial_validate_key(name, value, "output_options", OutputOptions) cls._partial_validate_key(name, value, "ytdl_options", YTDLOptions) cls._partial_validate_key(name, value, "overrides", Overrides) @@ -269,53 +158,6 @@ class Preset(_PresetShell): def _source_variables(self) -> List[str]: return Entry.source_variables() - def __validate_and_get_downloader(self, downloader_source: str) -> Type[SourcePlugin]: - return self._validate_key(key=downloader_source, validator=DownloadStrategyValidator).get( - downloader_source=downloader_source - ) - - def __validate_and_get_downloader_options( - self, downloader_source: str, downloader: Type[SourcePlugin] - ) -> OptionsValidator: - # Remove the download_strategy key before validating it against the downloader options - # TODO: make this cleaner - del self._dict[downloader_source]["download_strategy"] - return self._validate_key(key=downloader_source, validator=downloader.plugin_options_type) - - def __validate_and_get_downloader_and_options( - self, - ) -> Tuple[Type[SourcePlugin], OptionsValidator]: - downloader: Optional[Type[SourcePlugin]] = None - download_options: Optional[OptionsValidator] = None - downloader_sources = DownloadStrategyMapping.sources() - - for key in self._keys: - # skip if the key is not a download source - if key not in downloader_sources: - continue - - # Ensure there are not multiple sources, i.e. youtube and soundcloud - if downloader: - raise self._validation_exception( - f"'{self._name}' can only have one of the following sources: " - f"{', '.join(downloader_sources)}" - ) - - downloader = self.__validate_and_get_downloader(downloader_source=key) - download_options = self.__validate_and_get_downloader_options( - downloader_source=key, downloader=downloader - ) - - # If downloader was not set, error since it is required - if not downloader: - - raise self._validation_exception( - f"'{self._name} must have one of the following sources: " - f"{', '.join(downloader_sources)}" - ) - - return downloader, download_options - def __validate_and_get_plugins(self) -> PresetPlugins: preset_plugins = PresetPlugins() @@ -474,7 +316,9 @@ class Preset(_PresetShell): # Perform the merge of parent presets before validating any keys self.__merge_parent_preset_dicts_if_present(config=config) - self.downloader, self.downloader_options = self.__validate_and_get_downloader_and_options() + self.downloader_options: MultiUrlValidator = self._validate_key( + key="download", validator=MultiUrlValidator + ) self.output_options = self._validate_key( key="output_options", diff --git a/src/ytdl_sub/config/preset_class_mappings.py b/src/ytdl_sub/config/preset_class_mappings.py index f3745d5c..060d8c89 100644 --- a/src/ytdl_sub/config/preset_class_mappings.py +++ b/src/ytdl_sub/config/preset_class_mappings.py @@ -3,8 +3,6 @@ from typing import List from typing import Type from ytdl_sub.config.plugin import Plugin -from ytdl_sub.downloaders.source_plugin import SourcePlugin -from ytdl_sub.downloaders.url.downloader import MultiUrlDownloader from ytdl_sub.plugins.audio_extract import AudioExtractPlugin from ytdl_sub.plugins.chapters import ChaptersPlugin from ytdl_sub.plugins.date_range import DateRangePlugin @@ -21,83 +19,6 @@ from ytdl_sub.plugins.subtitles import SubtitlesPlugin from ytdl_sub.plugins.video_tags import VideoTagsPlugin -class DownloadStrategyMapping: - """ - Maps downloader strategies defined in the preset to its respective downloader class - """ - - _MAPPING: Dict[str, Dict[str, Type[SourcePlugin]]] = { - "download": { - "multi_url": MultiUrlDownloader, - "url": MultiUrlDownloader, - }, - } - - @classmethod - def sources(cls) -> List[str]: - """ - Returns - ------- - Available download sources - """ - return sorted(list(cls._MAPPING.keys())) - - @classmethod - def _validate_is_source(cls, source: str) -> None: - """ - Ensure the source exists - """ - if source not in cls.sources(): - raise ValueError( - f"Tried to use source '{source}' which does not exist. Available sources: " - f"{', '.join(cls.sources())}" - ) - - @classmethod - def source_download_strategies(cls, source: str) -> List[str]: - """ - Parameters - ---------- - source - Name of the source - - Returns - ------- - Available download strategies for the given source - """ - cls._validate_is_source(source) - return sorted(list(cls._MAPPING[source].keys())) - - @classmethod - def _validate_is_download_strategy(cls, source: str, download_strategy: str) -> None: - """ - Ensure the download strategy for the given source exists - """ - if download_strategy not in cls.source_download_strategies(source): - raise ValueError( - f"Tried to use download strategy '{download_strategy}' with source '{source}', " - f"which does not exist. Available download strategies: " - f"{', '.join(cls.source_download_strategies(source))}" - ) - - @classmethod - def get(cls, source: str, download_strategy: str) -> Type[SourcePlugin]: - """ - Parameters - ---------- - source: - The source (i.e. 'youtube', 'soundcloud') of the downloader - download_strategy: - The download strategy name - - Returns - ------- - The downloader class - """ - cls._validate_is_download_strategy(source, download_strategy) - return cls._MAPPING[source][download_strategy] - - class PluginMapping: """ Maps plugins defined in the preset to its respective plugin class diff --git a/src/ytdl_sub/downloaders/url/downloader.py b/src/ytdl_sub/downloaders/url/downloader.py index 96c94888..d66749af 100644 --- a/src/ytdl_sub/downloaders/url/downloader.py +++ b/src/ytdl_sub/downloaders/url/downloader.py @@ -1,6 +1,5 @@ import contextlib import os -from abc import ABC from pathlib import Path from typing import Dict from typing import Iterable @@ -188,7 +187,7 @@ class UrlDownloaderCollectionVariablePlugin(SourcePluginExtension): return entry -class MultiUrlDownloader(SourcePlugin[MultiUrlValidator], ABC): +class MultiUrlDownloader(SourcePlugin[MultiUrlValidator]): """ Class that interacts with ytdl to perform the download of metadata and content, and should translate that to list of Entry objects. diff --git a/src/ytdl_sub/subscriptions/base_subscription.py b/src/ytdl_sub/subscriptions/base_subscription.py index 82c697b5..e5c38ca7 100644 --- a/src/ytdl_sub/subscriptions/base_subscription.py +++ b/src/ytdl_sub/subscriptions/base_subscription.py @@ -1,15 +1,13 @@ from abc import ABC from pathlib import Path -from typing import Type from ytdl_sub.config.config_validator import ConfigOptions from ytdl_sub.config.preset import Preset from ytdl_sub.config.preset import PresetPlugins -from ytdl_sub.config.preset_options import OptionsValidator from ytdl_sub.config.preset_options import OutputOptions from ytdl_sub.config.preset_options import Overrides from ytdl_sub.config.preset_options import YTDLOptions -from ytdl_sub.downloaders.source_plugin import SourcePlugin +from ytdl_sub.downloaders.url.validators import MultiUrlValidator from ytdl_sub.ytdl_additions.enhanced_download_archive import EnhancedDownloadArchive @@ -52,16 +50,7 @@ class BaseSubscription(ABC): ) @property - def downloader_class(self) -> Type[SourcePlugin]: - """ - Returns - ------- - This subscription's downloader class - """ - return self._preset_options.downloader - - @property - def downloader_options(self) -> OptionsValidator: + def downloader_options(self) -> MultiUrlValidator: """ Returns ------- diff --git a/src/ytdl_sub/subscriptions/subscription_download.py b/src/ytdl_sub/subscriptions/subscription_download.py index 25c97a09..b0de508d 100644 --- a/src/ytdl_sub/subscriptions/subscription_download.py +++ b/src/ytdl_sub/subscriptions/subscription_download.py @@ -11,6 +11,7 @@ from ytdl_sub.config.plugin import SplitPlugin from ytdl_sub.downloaders.info_json.info_json_downloader import InfoJsonDownloader from ytdl_sub.downloaders.info_json.info_json_downloader import InfoJsonDownloaderOptions from ytdl_sub.downloaders.source_plugin import SourcePlugin +from ytdl_sub.downloaders.url.downloader import MultiUrlDownloader from ytdl_sub.downloaders.ytdl_options_builder import YTDLOptionsBuilder from ytdl_sub.entries.entry import Entry from ytdl_sub.subscriptions.base_subscription import BaseSubscription @@ -330,7 +331,7 @@ class SubscriptionDownload(BaseSubscription, ABC): dry_run=dry_run, ) - downloader = self.downloader_class( + downloader = MultiUrlDownloader( options=self.downloader_options, enhanced_download_archive=self._enhanced_download_archive, download_ytdl_options=subscription_ytdl_options.download_builder(), @@ -368,7 +369,7 @@ class SubscriptionDownload(BaseSubscription, ABC): # Re-add the original downloader class' plugins plugins.extend( - self.downloader_class( + MultiUrlDownloader( options=self.downloader_options, enhanced_download_archive=self._enhanced_download_archive, download_ytdl_options=subscription_ytdl_options.download_builder(), diff --git a/src/ytdl_sub/subscriptions/subscription_ytdl_options.py b/src/ytdl_sub/subscriptions/subscription_ytdl_options.py index b4bdeb3e..4ce74409 100644 --- a/src/ytdl_sub/subscriptions/subscription_ytdl_options.py +++ b/src/ytdl_sub/subscriptions/subscription_ytdl_options.py @@ -7,7 +7,6 @@ from typing import TypeVar from ytdl_sub.config.plugin import Plugin from ytdl_sub.config.preset import Preset -from ytdl_sub.downloaders.source_plugin import SourcePlugin from ytdl_sub.downloaders.ytdl_options_builder import YTDLOptionsBuilder from ytdl_sub.plugins.audio_extract import AudioExtractPlugin from ytdl_sub.plugins.chapters import ChaptersPlugin @@ -42,10 +41,6 @@ class SubscriptionYTDLOptions: return plugin return None - @property - def _downloader(self) -> Type[SourcePlugin]: - return self._preset.downloader - @property def _global_options(self) -> Dict: """ diff --git a/tests/unit/config/test_config_file.py b/tests/unit/config/test_config_file.py index 0edfbd08..2966df44 100644 --- a/tests/unit/config/test_config_file.py +++ b/tests/unit/config/test_config_file.py @@ -6,7 +6,6 @@ import pytest from ytdl_sub.config.config_file import ConfigFile from ytdl_sub.config.preset import PRESET_KEYS -from ytdl_sub.config.preset_class_mappings import DownloadStrategyMapping from ytdl_sub.config.preset_class_mappings import PluginMapping from ytdl_sub.utils.exceptions import ValidationException @@ -57,11 +56,6 @@ class TestConfigFilePartiallyValidatesPresets: if plugin not in excluded_plugins: self._partial_validate({plugin: {}}) - @pytest.mark.parametrize("source", DownloadStrategyMapping.sources()) - def test_success__empty_sources(self, source: str): - for download_strategy in DownloadStrategyMapping.source_download_strategies(source): - self._partial_validate({source: {"download_strategy": download_strategy}}) - def test_error__bad_preset_section(self): self._partial_validate( preset_dict={"does_not_exist": "lol"}, @@ -78,21 +72,6 @@ class TestConfigFilePartiallyValidatesPresets: # "Contains the sources download, youtube but can only have one", # ) - def test_error__no_download_strategy(self): - self._partial_validate( - preset_dict={"download": {}}, - expected_error_message="Validation error in partial_preset.download: " - "missing the required field 'download_strategy'", - ) - - def test_error__bad_download_strategy(self): - self._partial_validate( - preset_dict={"download": {"download_strategy": "fail"}}, - expected_error_message="Validation error in partial_preset.download: " - "Tried to use download strategy 'fail' with source 'download', " - "which does not exist. Available download strategies: multi_url, url", - ) - def test_error__bad_download_strategy_args(self): self._partial_validate( preset_dict={"download": {"download_strategy": "multi_url", "bad_key": "nope"}}, @@ -186,25 +165,3 @@ class TestConfigFilePartiallyValidatesPresets: }, }, ) - - def test_partial_validate_partial_download_strategies_mismatch(self): - with pytest.raises( - ValidationException, - match=re.escape( - "Preset parent uses download strategy multi_url, but is inherited by preset child " - "which uses download strategy url." - ), - ): - _ = ConfigFile( - name="test_partial_validate", - value={ - "configuration": {"working_directory": "."}, - "presets": { - "parent": {"download": {"download_strategy": "multi_url"}}, - "child": { - "preset": "parent", - "download": {"download_strategy": "url", "url": "should work"}, - }, - }, - }, - )