From e82e30f389cdbd628f762c3c5744df2c69c05cbc Mon Sep 17 00:00:00 2001 From: gesh Date: Mon, 4 May 2026 17:20:49 +0300 Subject: [PATCH 1/5] feat: Use platformdirs for XDG support Instead of reinventing the wheel, just use the standard apis here. In fact, this was the approach introduced in 4f49e8ef2220126a054aa3bb041d728a01645c67, and the platformdirs dependency has stuck around since. This should have the added bonus of making our configuration more idiomatic in Windows and Mac. Furthermore, make tests use temp dirs for their configuration, and in particular check for both idiomatic and non-idiomatic (backwards-compatible) configurations. Finally, use this opportunity to split the directories used according to use (using XDG variables for reference): - Under XDG_CONFIG_HOME: config.json - Under XDG_CACHE_HOME: the spotify and spotipy caches and the temp dir - Under XDG_DATA_HOME: the ffmpeg binary and web-ui - Under XDG_STATE_HOME: the web sessions and the logs Also use the occasion to rename .spotipy -> spotipy and .spotify_cache -> spotify_cache -- there's no need to doubly hide such directories, given the spotify directories are likely to be hidden anyway. API changes (grepped the codebase to make sure there are no remaining references, but these can be reverted if needed): - Remove: spotdl.utils.config.get_spotdl_path This no longer makes sense, now spotdl's files aren't concentrated in a single directory - Rename: spotdl.utils.config.get_cache_path -> get_spotipy_client_cache_path Made the stylistic choice to reserve the generic names get_{config,cache,data,state}_path for the "roots" of those kinds of paths, and to otherwise name the functions after the directories' usecase. Reordered the functions accordingly. Avoided the obvious name of get_spotipy_cache_path to distance it from the existing get_spotify_cache_path. - Add: spotdl.utils.ffmpeg.get_local_ffmpeg_path Gives the path to where the local ffmpeg binary would be, regardless of whether it exists -- used for deduplicating logic in spotdl.utils.ffmpeg, see below. - Add: tests.conftest.config_paths, tests.conftest.config_dirs, tests.conftest.populate_config config_paths gives a single place where the list of all paths spotdl uses can be listed, along with their current default values. populate_config takes a set of paths and populates them with the default config -- this is used in eg test_config to populate the various permutations of configurations. We write the default config JSON to those directories so spotdl has something to work with. config_dirs sets up a temp dir configured for XDG with all the spotdl paths populated. This also permits cutting down on the use of monkeypatching in other tests -- see test_init, test_ffmpeg and test_metadata Drive-by improvements: - spotdl.utils.ffmpeg Deduplicate the logic in get_local_ffmpeg and download_ffmpeg for finding the path to the local ffmpeg. - spotdl.web.api Use more idiomatic path construction, and in particular use the / operator instead of embedding / in strings passed to the Path constructor (should be more reliable) Implementation notes: - spotdl.utils.config.get_path_fallback The current implementation of the file fallback picks its behaviours according to whether the parent directory exists. For our current purposes, this is sufficient (and necessary, given we want to signal when a config file is missing), but this does mean we forgo being able to move config files into subdirectories (eg `~/.spotdlrc` -> `~/.spotdl/spotdlrc`) - spotdl.utils.config.old_spotdl_dirs In order for this list to be affected by the monkeypatching to `os.environ` in tests, we need to wrap it in a function so it gets reevaluated every time. This way, it picks up the changes to the environment. A hack, but I see no better way around it. - test_config To be able to parametrize over the choice of fixtures, request.getfixturevalue needed to be used. This is slightly black magic, but it seems well-supported. Closes: #2058, #2387 Updates: #2514, #2540 --- docs/usage.md | 11 +- spotdl/console/web.py | 4 +- spotdl/utils/arguments.py | 8 +- spotdl/utils/config.py | 201 ++++++++++++++++++++++++----------- spotdl/utils/ffmpeg.py | 27 +++-- spotdl/utils/spotify.py | 4 +- spotdl/web/api.py | 8 +- spotdl/web/routes.py | 4 +- tests/conftest.py | 45 ++++++++ tests/test_init.py | 16 +-- tests/utils/test_config.py | 94 ++++++++++------ tests/utils/test_ffmpeg.py | 16 ++- tests/utils/test_metadata.py | 9 +- 13 files changed, 298 insertions(+), 149 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 0c6e5e213..21564bed2 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -237,8 +237,9 @@ add option `--web-use-output-dir`, which will make output directory follow `--ou ### Config file location -The config file is located at `C:\Users\user\.spotdl\config.json` -or `~/.config/spotdl/config.json` under Linux +The config file is located at `%USERPROFILE%\AppData\Local\spotdl\config.json` +or `$XDG_CONFIG_HOME/spotdl/config.json` under Linux (usually +`XDG_CONFIG_HOME=~/.config`) > Note: Prior to v4.4.3 the default Linux location was `~/.spotdl/config.json` which will be used if the new directory doesn't exist. @@ -273,7 +274,7 @@ If you don't want the config to load automatically, change the `load_config` opt "auth_token": null, "user_auth": false, "headless": false, - "cache_path": "/Users/username/.spotdl/.spotipy", + "cache_path": "/home/username/.cache/spotdl/spotipy", "no_cache": false, "max_retries": 3, "use_cache_file": false, @@ -407,7 +408,7 @@ Main options: to use `--generate-lrc` option. --genius-access-token GENIUS_TOKEN Lets you choose your own Genius access token. - --config Use the config file to download songs. It's located under C:\Users\user\.spotdl\config.json or ~/.spotdl/config.json under linux + --config Use the config file to download songs. It's located under %USERPROFILE%\AppData\Local\spotdl\config.json or $XDG_CONFIG_HOME/spotdl/config.json under linux --search-query SEARCH_QUERY The search query to use, available variables: {title}, {artists}, {artist}, {album}, {album-artist}, {genre}, {disc-number}, {disc-count}, {duration}, {year}, {original-date}, {track-number}, {tracks-count}, {isrc}, {track-id}, {publisher}, {list-length}, {list-position}, {list-name}, {output-ext} @@ -432,7 +433,7 @@ Spotify options: --max-retries MAX_RETRIES The maximum number of retries to perform when getting metadata. --headless Run in headless mode. - --use-cache-file Use the cache file to get metadata. It's located under C:\Users\\.spotdl\.spotify_cache or ~/.spotdl/.spotify_cache under linux. It only caches tracks and + --use-cache-file Use the cache file to get metadata. It's located under %USERPROFILE%\AppData\Local\spotdl\Cache\spotify_cache or $XDG_CACHE_HOME/spotdl/spotify_cache under linux. It only caches tracks and gets updated whenever spotDL gets metadata from Spotify. (It may provide outdated metadata use with caution) FFmpeg options: diff --git a/spotdl/console/web.py b/spotdl/console/web.py index 603c218b7..086a669a3 100644 --- a/spotdl/console/web.py +++ b/spotdl/console/web.py @@ -16,7 +16,7 @@ from spotdl._version import __version__ from spotdl.types.options import DownloaderOptions, WebOptions -from spotdl.utils.config import get_spotdl_path, get_web_ui_path +from spotdl.utils.config import get_state_path, get_web_ui_path from spotdl.utils.logging import NAME_TO_LEVEL from spotdl.utils.web import ( ALLOWED_ORIGINS, @@ -154,7 +154,7 @@ def handle_shutdown(signum, frame): # pylint: disable=unused-argument not app_state.web_settings["keep_sessions"] and not app_state.web_settings["web_use_output_dir"] ): - sessions_dir = Path(get_spotdl_path() / "web/sessions") + sessions_dir = Path(get_state_path() / "web" / "sessions") logger.info("Removing sessions directories") if sessions_dir.exists(): shutil.rmtree(sessions_dir) diff --git a/spotdl/utils/arguments.py b/spotdl/utils/arguments.py index 797c07aab..bd2c362d0 100644 --- a/spotdl/utils/arguments.py +++ b/spotdl/utils/arguments.py @@ -140,8 +140,8 @@ def parse_main_options(parser: _ArgumentGroup): action="store_true", help=( "Use the config file to download songs. " - "It's located under C:\\Users\\user\\.spotdl\\config.json " - "or ~/.spotdl/config.json under linux" + "It's located under %USERPROFILE%\\AppData\\Local\\spotdl\\config.json " + "or $XDG_CONFIG_HOME/spotdl/config.json under linux" ), ) @@ -252,8 +252,8 @@ def parse_spotify_options(parser: _ArgumentGroup): const=True, help=( "Use the cache file to get metadata. " - "It's located under C:\\Users\\user\\.spotdl\\.spotify_cache " - "or ~/.spotdl/.spotify_cache under linux. " + "It's located under %USERPROFILE%\\AppData\\Local\\spotdl\\Cache\\spotify_cache " + "or $XDG_CACHE_HOME/spotdl/spotify_cache under linux. " "It only caches tracks and " "gets updated whenever spotDL gets metadata from Spotify. " "(It may provide outdated metadata use with caution)" diff --git a/spotdl/utils/config.py b/spotdl/utils/config.py index 497a4452b..5e5bde451 100644 --- a/spotdl/utils/config.py +++ b/spotdl/utils/config.py @@ -6,11 +6,11 @@ import json import logging -import os import platform +import platformdirs as pd from argparse import Namespace from pathlib import Path -from typing import Any, Dict, Tuple, Union +from typing import Any, Dict, Tuple, Union, TypeVar from spotdl.types.options import ( DownloaderOptions, @@ -19,12 +19,15 @@ WebOptions, ) +T = TypeVar("T") + __all__ = [ "ConfigError", - "get_spotdl_path", "get_config_file", - "get_cache_path", + "get_state_path", "get_temp_path", + "get_utils_path", + "get_spotipy_client_cache_path", "get_errors_path", "get_web_ui_path", "get_config", @@ -45,91 +48,162 @@ class ConfigError(Exception): """ -def get_spotdl_path() -> Path: +def get_path_fallback(default: Path, *fallbacks: Path) -> Path: """ - Get the path to the spotdl folder, following XDG standards on Linux. - ~/.config/spotdl/ is used if it exists, else ~/.spotdl if it exists. - If the spotdl directory does not exist, it will be created + Get the first path among current and fallbacks that exists. + If none exist, create current and return it. + All the paths passed must refer to directories. ### Returns - - The path to the spotdl folder. + - A path, guaranteed to exist, the first such among default and fallbacks. + ### Note + - This has the implication that for fallbacks for files, we pick behaviours according to whether + their _parent directory_ exists. For our current purposes, this is sufficient, since we need + to be able to raise ConfigError if the config file doesn't exist, and are so far not moving + files into subdirectories of existing fallback paths. + (To clarify -- the identified failure mode is moving ~/.spotdlrc to ~/.spotdl/spotdlrc) + """ - ### Notes + for path in default, *fallbacks: + if path.exists(): + return path + default.mkdir(parents=True, exist_ok=True) + return default + + +def elemIf(x: T, p: bool) -> T: + """ + Yields x if p is true. + Useful for inserting x into a list only when p is true. """ + if p: + yield x + + +# Backwards-compatibility for paths under the old behaviour where spotdl would put everything under +# a "spotdl folder" and all paths would be relative to these root directories. +# +# In order for this list to be affected by the monkeypatching to `os.environ` in tests, we need to +# wrap it in a function so it gets reevaluated every time. +# This way, it picks up the changes to the environment. +def old_spotdl_dirs(): + return [ + # An initial attempt at XDG Base Directory support hardcoded the path as ~/.config/spotdl on + # linux + *(elemIf(Path.home() / ".config" / "spotdl", platform.system() == "Linux")), + # The previous behaviour, which unconditionally used the unixy ~/.spotdl + Path.home() / ".spotdl", + ] - # For Linux systems, we follow the XDG Base Directory Specification - if platform.system() == "Linux": - # Define the new, correct XDG config path (~/.config/spotdl) - xdg_config_path = Path.home() / ".config" / "spotdl" - # Define the old path (~/.spotdl) for backward compatibility - old_spotdl_path = Path.home() / ".spotdl" +def get_config_file() -> Path: + """ + Get config file path - # Scenario 1: The user already has the new XDG config folder. Use it. - if xdg_config_path.exists(): - return xdg_config_path + ### Returns + - The path to the config file. + """ - # Scenario 2: The user is an existing user with only the old folder. Use the old one. - if old_spotdl_path.exists(): - return old_spotdl_path + return ( + get_path_fallback( + *([pd.user_config_path(appname="spotdl")] + old_spotdl_dirs()) + ) + / "config.json" + ) - # Scenario 3: The user is brand new. Create and use the new XDG path. - os.makedirs(xdg_config_path, exist_ok=True) - return xdg_config_path - # For non-Linux systems (like Windows), use the default ~/.spotdl path - spotdl_path = Path.home() / ".spotdl" - os.makedirs(spotdl_path, exist_ok=True) +def get_state_path() -> Path: + """ + Get the path to the state folder. - return spotdl_path + ### Returns + - The path to the directory containing working state. + """ + return get_path_fallback( + *( + r / d + for r in [pd.user_state_path(appname="spotdl")] + old_spotdl_dirs() + for d in [""] + ) + ) -def get_config_file() -> Path: + +def get_temp_path() -> Path: """ - Get config file path + Get the path to the temp folder. ### Returns - - The path to the config file. + - The path to the temp folder. """ - return get_spotdl_path() / "config.json" + return get_path_fallback( + *( + r / d + for r in [pd.user_cache_path(appname="spotdl")] + old_spotdl_dirs() + for d in ["temp"] + ) + ) -def get_cache_path() -> Path: +def get_utils_path() -> Path: """ - Get the path to the cache folder. + Get the path to the utilities folder. ### Returns - - The path to the spotipy cache file. + - The path to the directory containing support programs. """ - return get_spotdl_path() / ".spotipy" + return get_path_fallback( + *( + [r / d for r in [pd.user_data_path(appname="spotdl")] for d in ["utils"]] + + [r / d for r in old_spotdl_dirs() for d in [""]] + ) + ) -def get_spotify_cache_path() -> Path: +def get_spotipy_client_cache_path() -> Path: """ - Get the path to the spotify cache folder. + Get the path to the cache folder. ### Returns - The path to the spotipy cache file. """ - return get_spotdl_path() / ".spotify_cache" + return get_path_fallback( + *( + [ + r / d + for r in [pd.user_cache_path(appname="spotdl")] + for d in ["spotipy", ".spotipy"] + ] + + [r / d for r in old_spotdl_dirs() for d in [".spotipy", "spotipy"]] + ) + ) -def get_temp_path() -> Path: +def get_spotify_cache_path() -> Path: """ - Get the path to the temp folder. + Get the path to the spotify cache folder. ### Returns - - The path to the temp folder. + - The path to the spotipy cache file. """ - temp_path = get_spotdl_path() / "temp" - if not temp_path.exists(): - os.mkdir(temp_path) - - return temp_path + return get_path_fallback( + *( + [ + r / d + for r in [pd.user_cache_path(appname="spotdl")] + for d in ["spotify_cache", ".spotify_cache"] + ] + + [ + r / d + for r in old_spotdl_dirs() + for d in [".spotify_cache", "spotify_cache"] + ] + ) + ) def get_errors_path() -> Path: @@ -143,12 +217,13 @@ def get_errors_path() -> Path: - If the errors directory does not exist, it will be created. """ - errors_path = get_spotdl_path() / "errors" - - if not errors_path.exists(): - os.mkdir(errors_path) - - return errors_path + return get_path_fallback( + *( + r / d + for r in [pd.user_log_path(appname="spotdl")] + old_spotdl_dirs() + for d in ["errors"] + ) + ) def get_web_ui_path() -> Path: @@ -162,14 +237,14 @@ def get_web_ui_path() -> Path: - If the web-ui directory does not exist, it will be created. """ - # web_ui_path = get_spotdl_path() / "web-ui" - # web_ui_path = get_spotdl_path() / "src" / "spotdl" / "web" / "static" - web_ui_path = Path(__file__).parent.parent / "web" / "static" - - if not web_ui_path.exists(): - os.mkdir(web_ui_path) - - return web_ui_path + return get_path_fallback( + pd.user_data_path(appname="spotdl") / "web" / "static", + *( + r / d + for r in old_spotdl_dirs() + for d in ["web-ui", "src" / "spotdl" / "web" / "static"] + ) + ) def get_config() -> Dict[str, Any]: @@ -312,7 +387,7 @@ def get_parameter(cls, key): "auth_token": None, "user_auth": False, "headless": False, - "cache_path": str(get_cache_path()), + "cache_path": str(get_spotipy_client_cache_path()), "no_cache": False, "max_retries": 3, "use_cache_file": False, diff --git a/spotdl/utils/ffmpeg.py b/spotdl/utils/ffmpeg.py index 8f530ac12..6415c1836 100644 --- a/spotdl/utils/ffmpeg.py +++ b/spotdl/utils/ffmpeg.py @@ -15,7 +15,7 @@ import requests -from spotdl.utils.config import get_spotdl_path +from spotdl.utils.config import get_utils_path from spotdl.utils.formatter import to_ms __all__ = [ @@ -182,18 +182,29 @@ def get_ffmpeg_version(ffmpeg: str = "ffmpeg") -> Tuple[Optional[float], Optiona return (version, build_year) -def get_local_ffmpeg() -> Optional[Path]: +def get_local_ffmpeg_path() -> Path: """ Get local ffmpeg binary path. ### Returns - - Path to ffmpeg binary or None if not found. + - Path to where the ffmpeg binary should be """ - ffmpeg_path = Path(get_spotdl_path()) / ( - "ffmpeg" + (".exe" if platform.system() == "Windows" else "") + os_name = platform.system().lower() + ffmpeg_path = get_utils_path() / ( + "ffmpeg" + (".exe" if os_name == "windows" else "") ) + return ffmpeg_path + +def get_local_ffmpeg() -> Optional[Path]: + """ + Get a path pointing to the local ffmpeg binary, or None if it doesn't exist. + + ### Returns + - Path to the local ffmpeg binary if it exists, None otherwise + """ + ffmpeg_path = get_local_ffmpeg_path() if ffmpeg_path.is_file(): return ffmpeg_path @@ -230,11 +241,7 @@ def download_ffmpeg() -> Path: if ffmpeg_url is None: raise FFmpegError("FFmpeg binary is not available for your system.") - ffmpeg_path = Path( - os.path.join( - get_spotdl_path(), "ffmpeg" + (".exe" if os_name == "windows" else "") - ) - ) + ffmpeg_path = get_local_ffmpeg_path() # Download binary and save it to a file in spotdl directory ffmpeg_binary = requests.get(ffmpeg_url, allow_redirects=True, timeout=10).content diff --git a/spotdl/utils/spotify.py b/spotdl/utils/spotify.py index d7669a213..639bf04ca 100644 --- a/spotdl/utils/spotify.py +++ b/spotdl/utils/spotify.py @@ -17,7 +17,7 @@ from spotipy.cache_handler import CacheFileHandler, MemoryCacheHandler from spotipy.oauth2 import SpotifyClientCredentials, SpotifyOAuth -from spotdl.utils.config import get_cache_path, get_spotify_cache_path +from spotdl.utils.config import get_spotipy_client_cache_path, get_spotify_cache_path __all__ = [ "SpotifyError", @@ -93,7 +93,7 @@ def init( # pylint: disable=bad-mcs-method-argument credential_manager = None cache_handler = ( - CacheFileHandler(cache_path or get_cache_path()) + CacheFileHandler(cache_path or get_spotipy_client_cache_path()) if not no_cache else MemoryCacheHandler() ) diff --git a/spotdl/web/api.py b/spotdl/web/api.py index 48d9e2986..cfc73e7b0 100644 --- a/spotdl/web/api.py +++ b/spotdl/web/api.py @@ -19,7 +19,7 @@ from spotdl.types.playlist import Playlist from spotdl.types.song import Song from spotdl.utils.arguments import create_parser -from spotdl.utils.config import get_spotdl_path +from spotdl.utils.config import get_state_path from spotdl.utils.github import RateLimitError, get_latest_version, get_status from spotdl.utils.search import get_search_results from spotdl.utils.web import ( @@ -141,7 +141,7 @@ async def shutdown_event(): and not app_state.web_settings["web_use_output_dir"] ): app_state.logger.info("Removing sessions directories") - sessions_dir = Path(get_spotdl_path(), "web/sessions") + sessions_dir = get_state_path() / "web" / "sessions" if sessions_dir.exists(): shutil.rmtree(sessions_dir) @@ -181,7 +181,7 @@ async def download_url( client.downloader.settings["output"] = client.downloader_settings["output"] else: client.downloader.settings["output"] = str( - (get_spotdl_path() / f"web/sessions/{client.client_id}").absolute() + (get_state_path() / "web" / "sessions" / client.client_id).absolute() ) try: @@ -225,7 +225,7 @@ async def download_file( - returns the file response, filename specified to return as attachment. """ - expected_path = str((get_spotdl_path() / "web/sessions").absolute()) + expected_path = str((get_state_path() / "web" / "sessions").absolute()) if state.web_settings.get("web_use_output_dir", False): expected_path = str( Path(client.downloader_settings["output"].split("{", 1)[0]).absolute() diff --git a/spotdl/web/routes.py b/spotdl/web/routes.py index 56e24f9f8..2d4cfbbb2 100644 --- a/spotdl/web/routes.py +++ b/spotdl/web/routes.py @@ -18,7 +18,7 @@ from spotdl._version import __version__ from spotdl.download.downloader import AUDIO_PROVIDERS, LYRICS_PROVIDERS from spotdl.types.song import Song -from spotdl.utils.config import get_spotdl_path +from spotdl.utils.config import get_state_path from spotdl.utils.ffmpeg import FFMPEG_FORMATS from spotdl.utils.search import get_search_results from spotdl.utils.web import Client, app_state, validate_search_term @@ -324,7 +324,7 @@ async def gen_download(signals: Signals): client.downloader.settings["output"] = client.downloader_settings["output"] else: client.downloader.settings["output"] = str( - (get_spotdl_path() / f"web/sessions/{client.client_id}").absolute() + (get_state_path() / "web" / "sessions" / client.client_id).absolute() ) try: diff --git a/tests/conftest.py b/tests/conftest.py index 4cdd3204a..f2bb86820 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,11 +1,14 @@ import re import subprocess +import json from pathlib import Path +import platformdirs import pytest from spotdl.download.downloader import Downloader from spotdl.utils import ffmpeg +from spotdl.utils.config import DEFAULT_CONFIG from spotdl.utils.logging import init_logging from spotdl.utils.spotify import SpotifyClient @@ -89,6 +92,48 @@ def patch_dependencies(mocker, monkeypatch): mocker.patch.object(Downloader, "download_multiple_songs", autospec=True) +def config_paths(): + pd=platformdirs + return { + 'config_file' : pd.user_config_path(appname='spotdl') / 'config.json', + "state_dir" : pd.user_state_path(appname='spotdl'), + "temp_dir" : pd.user_cache_path(appname='spotdl') / 'temp', + "utils_dir" : pd.user_data_path(appname='spotdl') / 'utils', + "spotipy_client_cache_dir": pd.user_cache_path(appname='spotdl') / 'spotipy', + "spotify_cache_dir" : pd.user_cache_path(appname='spotdl') / 'spotify_cache', + "errors_dir" : pd.user_log_path(appname='spotdl') / 'errors', + "web_ui_dir" : pd.user_data_path(appname='spotdl') / 'web-ui', + } + + +@pytest.fixture() +def config_dirs(tmp_path_factory, monkeypatch): + """ + A system with all spotdl directories set to temporary directories. + Use this especially for tests that write (eg temp files) to the filesystem. + """ + home = tmp_path_factory.mktemp('user-home') + monkeypatch.setenv('XDG_CONFIG_HOME', str(home / 'config')) + monkeypatch.setenv('XDG_CACHE_HOME', str(home / 'cache')) + monkeypatch.setenv('XDG_DATA_HOME', str(home / 'data')) + monkeypatch.setenv('XDG_STATE_HOME', str(home / 'state')) + monkeypatch.setenv('HOME', str(home)) + monkeypatch.setenv('USERPROFILE', str(home)) + paths = config_paths() + populate_config(paths) + +def populate_config(paths): + for p in filter(lambda k: k.endswith('_file'), paths): + f = paths[p] + f.parent.mkdir(parents=True, exist_ok=True) + f.touch() + for p in filter(lambda k: k.endswith('_dir'), paths): + d = paths[p] + d.mkdir(parents=True, exist_ok=True) + with open(paths['config_file'], 'w') as fp: + json.dump(DEFAULT_CONFIG, fp, indent=4) + + def clean_ansi_sequence(text): """ Remove ANSI escape sequences from text diff --git a/tests/test_init.py b/tests/test_init.py index fad1f8f06..a8d0b6396 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -1,8 +1,4 @@ -from types import SimpleNamespace -import pytest - -import spotdl.utils.config from spotdl import Spotdl from spotdl.types.song import Song from spotdl.utils.config import DEFAULT_CONFIG, DOWNLOADER_OPTIONS @@ -10,15 +6,7 @@ from tests.conftest import new_initialize -@pytest.fixture() -def setup(tmp_path, monkeypatch): - monkeypatch.setattr(spotdl.utils.config, "get_spotdl_path", lambda *_: tmp_path) - data = SimpleNamespace() - data.directory = tmp_path - yield data - - -def test_get_urls(monkeypatch): +def test_get_urls(config_dirs, monkeypatch): """ Tests if spotdl can be initialized correctly. """ @@ -48,7 +36,7 @@ def test_get_urls(monkeypatch): assert len(urls) == 1 -def test_download(setup, monkeypatch, tmpdir): +def test_download(config_dirs, monkeypatch, tmpdir): """ Tests if spotdl can be initialized correctly. """ diff --git a/tests/utils/test_config.py b/tests/utils/test_config.py index 0884fcbfc..d919709e9 100644 --- a/tests/utils/test_config.py +++ b/tests/utils/test_config.py @@ -1,57 +1,91 @@ import os -from pathlib import Path -from types import SimpleNamespace import pytest +from tests.conftest import config_paths, populate_config from spotdl.utils.config import * +from spotdl.utils.config import get_spotify_cache_path +# Fixtures to test various path setups. In the following, XDG refers to the XDG Base Directory +# specification @pytest.fixture() -def setup(tmpdir, monkeypatch): - monkeypatch.setattr(os.path, "expanduser", lambda *_: tmpdir) - data = SimpleNamespace() - data.directory = tmpdir - yield data - - -def test_get_spotdl_path(setup): +def fs_pre_xdg(tmp_path_factory, monkeypatch): """ - Tests that the spotdl path is created if it does not exist. + A system ignorant of XDG -- the paths are in legacy locations and the XDG variables are unset. """ - - assert get_spotdl_path() == Path(setup.directory, ".spotdl") - assert os.path.exists(os.path.join(setup.directory, ".spotdl")) + monkeypatch.delenv('XDG_CONFIG_HOME', raising=False) + monkeypatch.delenv('XDG_CACHE_HOME', raising=False) + monkeypatch.delenv('XDG_DATA_HOME', raising=False) + monkeypatch.delenv('XDG_STATE_HOME', raising=False) + home = tmp_path_factory.mktemp('user-home') + monkeypatch.setenv('HOME', str(home)) + (home / '.spotdl').mkdir() + paths = { + 'config_file': home / '.spotdl' / 'config.json', + "state_dir": home / '.spotdl', + "temp_dir": home / '.spotdl' / 'temp', + "utils_dir": home / '.spotdl', + "spotipy_client_cache_dir": home / '.spotdl' / '.spotipy', + "spotify_cache_dir": home / '.spotdl' / '.spotify_cache', + "errors_dir": home / '.spotdl' / 'errors', + "web_ui_dir": home / '.spotdl' / 'web-ui', + } + populate_config(paths) + return paths -def test_get_config_path(setup): +@pytest.fixture() +def fs_xdg_new(tmp_path_factory, monkeypatch): """ - Tests if the path to config file is correct. + A system of an XDG-using user who hasn't yet created their spotdl directories. """ + home = tmp_path_factory.mktemp('user-home') + monkeypatch.setenv('XDG_CONFIG_HOME', str(home / 'config')) + monkeypatch.setenv('XDG_CACHE_HOME', str(home / 'cache')) + monkeypatch.setenv('XDG_DATA_HOME', str(home / 'data')) + monkeypatch.setenv('XDG_STATE_HOME', str(home / 'state')) + monkeypatch.setenv('HOME', str(home)) + return config_paths() - assert get_config_file() == Path(setup.directory, ".spotdl", "config.json") - - -def test_get_cache_path(setup): - """ - Tests if the path to the cache file is correct. - """ - assert get_cache_path() == Path(setup.directory, ".spotdl", ".spotipy") +@pytest.fixture() +def fs_xdg_full(config_dirs): + return config_paths() -def test_get_temp_path(setup): +@pytest.mark.parametrize('fs', ['fs_pre_xdg', 'fs_xdg_new', 'fs_xdg_full']) +def test_config_paths(fs, request): """ - Tests if the path to the temp folder is correct. + Tests that all config paths point to the expected locations """ - assert get_temp_path() == Path(setup.directory, ".spotdl", "temp") - - -def test_get_config_not_created(setup): + paths = request.getfixturevalue(fs) + funcs = { + 'config_file' : get_config_file, + "state_dir" : get_state_path, + "temp_dir" : get_temp_path, + "utils_dir" : get_utils_path, + "spotipy_client_cache_dir": get_spotipy_client_cache_path, + "spotify_cache_dir" : get_spotify_cache_path, + "errors_dir" : get_errors_path, + "web_ui_dir" : get_web_ui_path, + } + for k in funcs: + assert paths[k] == funcs[k]() + + +@pytest.mark.parametrize('fs', ['fs_pre_xdg', 'fs_xdg_new', 'fs_xdg_full']) +def test_get_config_not_created(fs, request): """ Tests if exception is raised if config file does not exist. """ + paths = request.getfixturevalue(fs) + try: + os.remove(paths['config_file']) + except FileNotFoundError: + pass + with pytest.raises(ConfigError): get_config() diff --git a/tests/utils/test_ffmpeg.py b/tests/utils/test_ffmpeg.py index 84b210fd9..01daf1a78 100644 --- a/tests/utils/test_ffmpeg.py +++ b/tests/utils/test_ffmpeg.py @@ -101,23 +101,21 @@ def test_get_local_ffmpeg(monkeypatch): assert str(local_ffmpeg).endswith("ffmpeg.exe") -def test_download_ffmpeg(monkeypatch, tmpdir): +def test_download_ffmpeg(config_dirs): """ Test download_ffmpeg function. """ - monkeypatch.setattr(spotdl.utils.ffmpeg, "get_spotdl_path", lambda *_: tmpdir) - assert download_ffmpeg() is not None -def test_convert(tmpdir, monkeypatch): +def test_convert(config_dirs): """ Test convert function. """ - monkeypatch.chdir(tmpdir) - monkeypatch.setattr(spotdl.utils.ffmpeg, "get_spotdl_path", lambda *_: tmpdir) + home = Path(os.environ['HOME']) + os.chdir(home) yt = YoutubeDL( { @@ -134,12 +132,12 @@ def test_convert(tmpdir, monkeypatch): assert convert( input_file=(download_info["url"], download_info["ext"]), - output_file=Path(tmpdir, "test.mp3"), + output_file=home / "test.mp3", ) == (True, None) assert convert( - input_file=Path(tmpdir, "test.mp3"), - output_file=Path(tmpdir, "test.m4a"), + input_file=home / "test.mp3", + output_file=home / "test.m4a", output_format="m4a", bitrate="320K", ) == (True, None) diff --git a/tests/utils/test_metadata.py b/tests/utils/test_metadata.py index 083bf7fb2..6edc5e352 100644 --- a/tests/utils/test_metadata.py +++ b/tests/utils/test_metadata.py @@ -1,3 +1,4 @@ +import os from pathlib import Path import pytest @@ -20,13 +21,13 @@ "m4a", ], ) -def test_embed_metadata(tmpdir, monkeypatch, output_format): +def test_embed_metadata(config_dirs, output_format): """ Test convert function. """ - monkeypatch.chdir(tmpdir) - monkeypatch.setattr(spotdl.utils.ffmpeg, "get_spotdl_path", lambda *_: tmpdir) + home = Path(os.environ['HOME']) + os.chdir(home) youtube = YoutubeDL( { @@ -67,7 +68,7 @@ def test_embed_metadata(tmpdir, monkeypatch, output_format): } song = Song.from_dict(song_obj) - output_file = Path(tmpdir / f"test.{output_format}") + output_file = home / f"test.{output_format}" assert download_info is not None assert convert( From c5e3f252248e247d5671471792b5077bf273917d Mon Sep 17 00:00:00 2001 From: gesh Date: Sun, 24 May 2026 21:26:33 +0300 Subject: [PATCH 2/5] Use more descriptive names --- spotdl/utils/config.py | 60 +++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/spotdl/utils/config.py b/spotdl/utils/config.py index 5e5bde451..c3a428e21 100644 --- a/spotdl/utils/config.py +++ b/spotdl/utils/config.py @@ -122,9 +122,9 @@ def get_state_path() -> Path: return get_path_fallback( *( - r / d - for r in [pd.user_state_path(appname="spotdl")] + old_spotdl_dirs() - for d in [""] + root / dir + for root in [pd.user_state_path(appname="spotdl")] + old_spotdl_dirs() + for dir in [""] ) ) @@ -139,9 +139,9 @@ def get_temp_path() -> Path: return get_path_fallback( *( - r / d - for r in [pd.user_cache_path(appname="spotdl")] + old_spotdl_dirs() - for d in ["temp"] + root / dir + for root in [pd.user_cache_path(appname="spotdl")] + old_spotdl_dirs() + for dir in ["temp"] ) ) @@ -156,8 +156,16 @@ def get_utils_path() -> Path: return get_path_fallback( *( - [r / d for r in [pd.user_data_path(appname="spotdl")] for d in ["utils"]] - + [r / d for r in old_spotdl_dirs() for d in [""]] + [ + root / dir + for root in [pd.user_data_path(appname="spotdl")] + for dir in ["utils"] + ] + + [ + root / dir + for root in old_spotdl_dirs() + for dir in [""] + ] ) ) @@ -173,11 +181,15 @@ def get_spotipy_client_cache_path() -> Path: return get_path_fallback( *( [ - r / d - for r in [pd.user_cache_path(appname="spotdl")] - for d in ["spotipy", ".spotipy"] + root / dir + for root in [pd.user_cache_path(appname="spotdl")] + for dir in ["spotipy", ".spotipy"] + ] + + [ + root / dir + for root in old_spotdl_dirs() + for dir in [".spotipy", "spotipy"] ] - + [r / d for r in old_spotdl_dirs() for d in [".spotipy", "spotipy"]] ) ) @@ -193,14 +205,14 @@ def get_spotify_cache_path() -> Path: return get_path_fallback( *( [ - r / d - for r in [pd.user_cache_path(appname="spotdl")] - for d in ["spotify_cache", ".spotify_cache"] + root / dir + for root in [pd.user_cache_path(appname="spotdl")] + for dir in ["spotify_cache", ".spotify_cache"] ] + [ - r / d - for r in old_spotdl_dirs() - for d in [".spotify_cache", "spotify_cache"] + root / dir + for root in old_spotdl_dirs() + for dir in [".spotify_cache", "spotify_cache"] ] ) ) @@ -219,9 +231,9 @@ def get_errors_path() -> Path: return get_path_fallback( *( - r / d - for r in [pd.user_log_path(appname="spotdl")] + old_spotdl_dirs() - for d in ["errors"] + root / dir + for root in [pd.user_log_path(appname="spotdl")] + old_spotdl_dirs() + for dir in ["errors"] ) ) @@ -240,9 +252,9 @@ def get_web_ui_path() -> Path: return get_path_fallback( pd.user_data_path(appname="spotdl") / "web" / "static", *( - r / d - for r in old_spotdl_dirs() - for d in ["web-ui", "src" / "spotdl" / "web" / "static"] + root / dir + for root in old_spotdl_dirs() + for dir in ["web-ui", "src" / "spotdl" / "web" / "static"] ) ) From 6ed7f34e8f8304ef8aed0c53242e7104b2cb27d0 Mon Sep 17 00:00:00 2001 From: gesh Date: Sun, 24 May 2026 21:46:57 +0300 Subject: [PATCH 3/5] Fix path handling As pointed out by Copilot: - / is only supported for Paths, not strs, cast first to get access - The fallback logic for the config file path was branching based on whether the config _directory_ existed, not the config file. Co-Authored-By: Copilot --- spotdl/utils/config.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/spotdl/utils/config.py b/spotdl/utils/config.py index c3a428e21..c3f86325e 100644 --- a/spotdl/utils/config.py +++ b/spotdl/utils/config.py @@ -104,12 +104,13 @@ def get_config_file() -> Path: - The path to the config file. """ - return ( - get_path_fallback( - *([pd.user_config_path(appname="spotdl")] + old_spotdl_dirs()) + return get_path_fallback( + *( + root / file + for root in [pd.user_config_path(appname="spotdl")] + old_spotdl_dirs() + for file in ["config.json"] + ) ) - / "config.json" - ) def get_state_path() -> Path: @@ -254,7 +255,7 @@ def get_web_ui_path() -> Path: *( root / dir for root in old_spotdl_dirs() - for dir in ["web-ui", "src" / "spotdl" / "web" / "static"] + for dir in ["web-ui", Path("src") / "spotdl" / "web" / "static"] ) ) From f40334d4b51ab77a660acee337c9f57fec9a681a Mon Sep 17 00:00:00 2001 From: gesh Date: Sun, 24 May 2026 21:55:27 +0300 Subject: [PATCH 4/5] Correct type hint As pointed out by Copilot, the type hint is hiding the fact that it returns an iterable, not a raw value. Also fix style. Co-Authored-By: Copilot --- spotdl/utils/config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spotdl/utils/config.py b/spotdl/utils/config.py index c3f86325e..c025e0519 100644 --- a/spotdl/utils/config.py +++ b/spotdl/utils/config.py @@ -71,7 +71,7 @@ def get_path_fallback(default: Path, *fallbacks: Path) -> Path: return default -def elemIf(x: T, p: bool) -> T: +def elem_if(x: T, p: bool) -> Iterable[T]: """ Yields x if p is true. Useful for inserting x into a list only when p is true. @@ -90,7 +90,7 @@ def old_spotdl_dirs(): return [ # An initial attempt at XDG Base Directory support hardcoded the path as ~/.config/spotdl on # linux - *(elemIf(Path.home() / ".config" / "spotdl", platform.system() == "Linux")), + *(elem_if(Path.home() / ".config" / "spotdl", platform.system() == "Linux")), # The previous behaviour, which unconditionally used the unixy ~/.spotdl Path.home() / ".spotdl", ] From ebdbeb471b5abac3c0181eecf431c26a94a7d29f Mon Sep 17 00:00:00 2001 From: gesh Date: Sun, 24 May 2026 21:56:35 +0300 Subject: [PATCH 5/5] Use monkeypatching to set test dir Instead of calling os.chdir within the test itself, which might leak between tests. Co-Authored-By: Copilot --- tests/conftest.py | 1 + tests/utils/test_ffmpeg.py | 3 --- tests/utils/test_metadata.py | 3 --- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index f2bb86820..875103a38 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -121,6 +121,7 @@ def config_dirs(tmp_path_factory, monkeypatch): monkeypatch.setenv('USERPROFILE', str(home)) paths = config_paths() populate_config(paths) + monkeypatch.chdir(home) def populate_config(paths): for p in filter(lambda k: k.endswith('_file'), paths): diff --git a/tests/utils/test_ffmpeg.py b/tests/utils/test_ffmpeg.py index 01daf1a78..1283511f2 100644 --- a/tests/utils/test_ffmpeg.py +++ b/tests/utils/test_ffmpeg.py @@ -114,9 +114,6 @@ def test_convert(config_dirs): Test convert function. """ - home = Path(os.environ['HOME']) - os.chdir(home) - yt = YoutubeDL( { "format": "bestaudio", diff --git a/tests/utils/test_metadata.py b/tests/utils/test_metadata.py index 6edc5e352..533b81560 100644 --- a/tests/utils/test_metadata.py +++ b/tests/utils/test_metadata.py @@ -26,9 +26,6 @@ def test_embed_metadata(config_dirs, output_format): Test convert function. """ - home = Path(os.environ['HOME']) - os.chdir(home) - youtube = YoutubeDL( { "format": "bestaudio",