[REFACTOR] No more download strategy (#669)

* [REFACTOR] No more download strategy

* remove more

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(
+        cls._partial_validate_key(name, value, "download", MultiUrlValidator)
-            config=config, name=name, value=value, parent_presets=parent_presets
-        )
         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",

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

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.

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]:
+    def downloader_options(self) -> MultiUrlValidator:
-        """
-        Returns
-        -------
-        This subscription's downloader class
-        """
-        return self._preset_options.downloader
-
-    @property
-    def downloader_options(self) -> OptionsValidator:
         """
         Returns
         -------

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(),

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:
         """

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"},
-                        },
-                    },
-                },
-            )