From 5c76d9a9d68690d123b2465cb6d5e2178f88cc72 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Mon, 22 Jan 2024 20:38:15 -0800 Subject: [PATCH] Addressed a bunch of TODO comments --- ideas.md | 3 + .../backends/yt_dlp/command_runner.ex | 2 +- .../downloader/backends/yt_dlp/video.ex | 23 +++----- lib/pinchflat/downloader/video_downloader.ex | 11 ++-- lib/pinchflat/profiles.ex | 58 ++----------------- lib/pinchflat/profiles/media_profile.ex | 4 ++ .../profiles/options/yt_dlp/option_builder.ex | 9 ++- .../options/yt_dlp/output_path_builder.ex | 10 ++-- .../yt_dlp/output_path_builder_test.exs | 25 ++++++++ .../backends/yt_dlp/video_collection_test.exs | 2 +- .../downloader/backends/yt_dlp/video_test.exs | 44 ++++++++++++++ .../downloader/video_downloader_test.exs | 35 +++++++++++ .../options/yt_dlp/option_builder_test.exs | 18 ++++++ test/pinchflat/utils/string_utils_test.exs | 2 +- 14 files changed, 158 insertions(+), 88 deletions(-) create mode 100644 ideas.md create mode 100644 test/pinchflat/downloader/backends/yt_dlp/output_path_builder_test.exs create mode 100644 test/pinchflat/downloader/backends/yt_dlp/video_test.exs create mode 100644 test/pinchflat/downloader/video_downloader_test.exs create mode 100644 test/pinchflat/profiles/options/yt_dlp/option_builder_test.exs diff --git a/ideas.md b/ideas.md new file mode 100644 index 0000000..fabc001 --- /dev/null +++ b/ideas.md @@ -0,0 +1,3 @@ +- Write media datbase ID as metadata/to file/whatever so it gives us an option to retroactively match media to the DB down the line. Useful if someone moves the media without informing the UI + - Use a UUID for the media database ID (or at least alongside it) +- Look into this and its recommended plugins https://hexdocs.pm/ex_check/readme.html diff --git a/lib/pinchflat/downloader/backends/yt_dlp/command_runner.ex b/lib/pinchflat/downloader/backends/yt_dlp/command_runner.ex index 522c043..b452dfd 100644 --- a/lib/pinchflat/downloader/backends/yt_dlp/command_runner.ex +++ b/lib/pinchflat/downloader/backends/yt_dlp/command_runner.ex @@ -11,7 +11,7 @@ defmodule Pinchflat.Downloader.Backends.YtDlp.CommandRunner do @doc """ Runs a yt-dlp command and returns the string output - # TODO: deduplicate command opts, keeping the last one on conflict + # IDEA: deduplicate command opts, keeping the last one on conflict although possibly not needed (and a LOT easier) if yt-dlp just ignores duplicate options (ie: look into that) """ diff --git a/lib/pinchflat/downloader/backends/yt_dlp/video.ex b/lib/pinchflat/downloader/backends/yt_dlp/video.ex index e00bec7..5d95e21 100644 --- a/lib/pinchflat/downloader/backends/yt_dlp/video.ex +++ b/lib/pinchflat/downloader/backends/yt_dlp/video.ex @@ -4,23 +4,16 @@ defmodule Pinchflat.Downloader.Backends.YtDlp.Video do """ @doc """ - Downloads a single video (and possible metadata) to the tmp directory. - Videos are downloaded in the following format: - `tmp/yt-dlp//.` - - The video will be moved to its final destination... elsewhere - # TODO: update these docs when I figure out a module to move videos - # TODO: test - # NOTE: maybe instead of moving it to the tempdir, I can just download it - to the final destination by using the `output` option. The - parser could be updated to generate a value for the output option. - This way, advanced users can just the yt-dlp output syntax and - newer users can use the easier liquid-like syntax. + Downloads a single video (and possible metadata) directly to its + final destination. Returns the parsed JSON output from yt-dlp. """ def download(url, command_opts \\ []) do - # TODO: if this stays this simple, consider not abstracting it - # HOWEVER - this module does provide clarity of intent so maybe keep? - backend_runner().run(url, command_opts) + opts = [:no_simulate, :dump_json] ++ command_opts + + case backend_runner().run(url, opts) do + {:ok, output} -> Phoenix.json_library().decode(output) + err -> err + end end defp backend_runner do diff --git a/lib/pinchflat/downloader/video_downloader.ex b/lib/pinchflat/downloader/video_downloader.ex index aeda97a..d5166b7 100644 --- a/lib/pinchflat/downloader/video_downloader.ex +++ b/lib/pinchflat/downloader/video_downloader.ex @@ -8,16 +8,15 @@ defmodule Pinchflat.Downloader.VideoDownloader do it open-ish for future expansion (just in case). """ + alias Pinchflat.Profiles.MediaProfile + alias Pinchflat.Downloader.Backends.YtDlp.Video, as: YtDlpVideo alias Pinchflat.Profiles.Options.YtDlp.OptionBuilder, as: YtDlpOptionBuilder @doc """ Downloads a single video based on the settings in the given media profile. - - # TODO: implement media profiles - so far this is a glorified mock - # TODO: test """ - def download_for_media_profile(url, media_profile, backend \\ :yt_dlp) do + def download_for_media_profile(url, %MediaProfile{} = media_profile, backend \\ :yt_dlp) do option_builder = option_builder(backend) video_backend = video_backend(backend) {:ok, options} = option_builder.build(media_profile) @@ -25,13 +24,13 @@ defmodule Pinchflat.Downloader.VideoDownloader do video_backend.download(url, options) end - def option_builder(backend) do + defp option_builder(backend) do case backend do :yt_dlp -> YtDlpOptionBuilder end end - def video_backend(backend) do + defp video_backend(backend) do case backend do :yt_dlp -> YtDlpVideo end diff --git a/lib/pinchflat/profiles.ex b/lib/pinchflat/profiles.ex index 2b58ee8..856f931 100644 --- a/lib/pinchflat/profiles.ex +++ b/lib/pinchflat/profiles.ex @@ -9,13 +9,7 @@ defmodule Pinchflat.Profiles do alias Pinchflat.Profiles.MediaProfile @doc """ - Returns the list of media_profiles. - - ## Examples - - iex> list_media_profiles() - [%MediaProfile{}, ...] - + Returns the list of media_profiles. Returns [%MediaProfile{}, ...] """ def list_media_profiles do Repo.all(MediaProfile) @@ -24,30 +18,12 @@ defmodule Pinchflat.Profiles do @doc """ Gets a single media_profile. - Raises `Ecto.NoResultsError` if the Media profile does not exist. - - ## Examples - - iex> get_media_profile!(123) - %MediaProfile{} - - iex> get_media_profile!(456) - ** (Ecto.NoResultsError) - + Returns %MediaProfile{}. Raises `Ecto.NoResultsError` if the Media profile does not exist. """ def get_media_profile!(id), do: Repo.get!(MediaProfile, id) @doc """ - Creates a media_profile. - - ## Examples - - iex> create_media_profile(%{field: value}) - {:ok, %MediaProfile{}} - - iex> create_media_profile(%{field: bad_value}) - {:error, %Ecto.Changeset{}} - + Creates a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}} """ def create_media_profile(attrs \\ %{}) do %MediaProfile{} @@ -56,16 +32,7 @@ defmodule Pinchflat.Profiles do end @doc """ - Updates a media_profile. - - ## Examples - - iex> update_media_profile(media_profile, %{field: new_value}) - {:ok, %MediaProfile{}} - - iex> update_media_profile(media_profile, %{field: bad_value}) - {:error, %Ecto.Changeset{}} - + Updates a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}} """ def update_media_profile(%MediaProfile{} = media_profile, attrs) do media_profile @@ -74,16 +41,7 @@ defmodule Pinchflat.Profiles do end @doc """ - Deletes a media_profile. - - ## Examples - - iex> delete_media_profile(media_profile) - {:ok, %MediaProfile{}} - - iex> delete_media_profile(media_profile) - {:error, %Ecto.Changeset{}} - + Deletes a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}} """ def delete_media_profile(%MediaProfile{} = media_profile) do Repo.delete(media_profile) @@ -91,12 +49,6 @@ defmodule Pinchflat.Profiles do @doc """ Returns an `%Ecto.Changeset{}` for tracking media_profile changes. - - ## Examples - - iex> change_media_profile(media_profile) - %Ecto.Changeset{data: %MediaProfile{}} - """ def change_media_profile(%MediaProfile{} = media_profile, attrs \\ %{}) do MediaProfile.changeset(media_profile, attrs) diff --git a/lib/pinchflat/profiles/media_profile.ex b/lib/pinchflat/profiles/media_profile.ex index 13dd0e4..ed66992 100644 --- a/lib/pinchflat/profiles/media_profile.ex +++ b/lib/pinchflat/profiles/media_profile.ex @@ -1,4 +1,8 @@ defmodule Pinchflat.Profiles.MediaProfile do + @moduledoc """ + A media profile is a set of settings that can be applied to many media sources + """ + use Ecto.Schema import Ecto.Changeset diff --git a/lib/pinchflat/profiles/options/yt_dlp/option_builder.ex b/lib/pinchflat/profiles/options/yt_dlp/option_builder.ex index 02ca39f..ead342f 100644 --- a/lib/pinchflat/profiles/options/yt_dlp/option_builder.ex +++ b/lib/pinchflat/profiles/options/yt_dlp/option_builder.ex @@ -2,20 +2,19 @@ defmodule Pinchflat.Profiles.Options.YtDlp.OptionBuilder do @moduledoc """ Builds the options for yt-dlp based on the given media profile. - TODO: probably make this a behaviour so I can add other backends later + IDEA: consider making this a behaviour so I can add other backends later """ + alias Pinchflat.Profiles.MediaProfile alias Pinchflat.Profiles.Options.YtDlp.OutputPathBuilder @doc """ Builds the options for yt-dlp based on the given media profile. - TODO: add a guard to ensure argument is a media profile - TODO: consider adding the ability to pass in a second argument to override + IDEA: consider adding the ability to pass in a second argument to override these options - TODO: test """ - def build(media_profile) do + def build(%MediaProfile{} = media_profile) do {:ok, output_path} = OutputPathBuilder.build(media_profile.output_path_template) # NOTE: I'll be hardcoding most things for now (esp. options to help me test) - diff --git a/lib/pinchflat/profiles/options/yt_dlp/output_path_builder.ex b/lib/pinchflat/profiles/options/yt_dlp/output_path_builder.ex index 17ef6d3..b058784 100644 --- a/lib/pinchflat/profiles/options/yt_dlp/output_path_builder.ex +++ b/lib/pinchflat/profiles/options/yt_dlp/output_path_builder.ex @@ -2,7 +2,7 @@ defmodule Pinchflat.Profiles.Options.YtDlp.OutputPathBuilder do @moduledoc """ Builds yt-dlp-friendly output paths for downloaded media - TODO: probably make this a behaviour so I can add other backends later + IDEA: consider making this a behaviour so I can add other backends later """ alias Pinchflat.RenderedString.Parser, as: TemplateParser @@ -12,8 +12,6 @@ defmodule Pinchflat.Profiles.Options.YtDlp.OutputPathBuilder do Translates liquid-style templates into yt-dlp-style templates, leaving yt-dlp syntax intact. - - TODO: test """ def build(template_string) do TemplateParser.parse(template_string, full_yt_dlp_options_map()) @@ -58,9 +56,9 @@ defmodule Pinchflat.Profiles.Options.YtDlp.OutputPathBuilder do defp custom_yt_dlp_option_map do %{ # Individual parts of the upload date - "upload_year" => "%(upload_date>%Y)s", - "upload_month" => "%(upload_date>%m)s", - "upload_day" => "%(upload_date>%d)s" + "upload_year" => "%(upload_date>%Y)S", + "upload_month" => "%(upload_date>%m)S", + "upload_day" => "%(upload_date>%d)S" } end end diff --git a/test/pinchflat/downloader/backends/yt_dlp/output_path_builder_test.exs b/test/pinchflat/downloader/backends/yt_dlp/output_path_builder_test.exs new file mode 100644 index 0000000..4098726 --- /dev/null +++ b/test/pinchflat/downloader/backends/yt_dlp/output_path_builder_test.exs @@ -0,0 +1,25 @@ +defmodule Pinchflat.Downloader.Backends.YtDlp.OutputPathBuilderTest do + use ExUnit.Case, async: true + + alias Pinchflat.Profiles.Options.YtDlp.OutputPathBuilder + + describe "build/1" do + test "it expands 'standard' curly brace variables in the template" do + assert {:ok, res} = OutputPathBuilder.build("/videos/{{ title }}.{{ ext }}") + + assert res == "/videos/%(title)S.%(ext)S" + end + + test "it expands 'custom' curly brace variables in the template" do + assert {:ok, res} = OutputPathBuilder.build("/videos/{{ upload_year }}.{{ ext }}") + + assert res == "/videos/%(upload_date>%Y)S.%(ext)S" + end + + test "it leaves yt-dlp variables alone" do + assert {:ok, res} = OutputPathBuilder.build("/videos/%(title)s.%(ext)s") + + assert res == "/videos/%(title)s.%(ext)s" + end + end +end diff --git a/test/pinchflat/downloader/backends/yt_dlp/video_collection_test.exs b/test/pinchflat/downloader/backends/yt_dlp/video_collection_test.exs index 31cb840..06c55e8 100644 --- a/test/pinchflat/downloader/backends/yt_dlp/video_collection_test.exs +++ b/test/pinchflat/downloader/backends/yt_dlp/video_collection_test.exs @@ -2,7 +2,7 @@ defmodule Pinchflat.Downloader.Backends.YtDlp.VideoCollectionTest do use ExUnit.Case, async: true import Mox - alias Pinchflat.Downloader.Backends.YtDlp.VideoCollection, as: VideoCollection + alias Pinchflat.Downloader.Backends.YtDlp.VideoCollection @channel_url "https://www.youtube.com/@TheUselessTrials" diff --git a/test/pinchflat/downloader/backends/yt_dlp/video_test.exs b/test/pinchflat/downloader/backends/yt_dlp/video_test.exs new file mode 100644 index 0000000..00a6430 --- /dev/null +++ b/test/pinchflat/downloader/backends/yt_dlp/video_test.exs @@ -0,0 +1,44 @@ +defmodule Pinchflat.Downloader.Backends.YtDlp.VideoTest do + use ExUnit.Case, async: true + import Mox + + alias Pinchflat.Downloader.Backends.YtDlp.Video + + @video_url "https://www.youtube.com/watch?v=TiZPUDkDYbk" + + setup :verify_on_exit! + + describe "download/2" do + test "it calls the backend runner with the expected arguments" do + expect(CommandRunnerMock, :run, fn @video_url, opts -> + assert opts == [:no_simulate, :dump_json] + + {:ok, "{}"} + end) + + assert {:ok, _} = Video.download(@video_url) + end + + test "it passes along additional options" do + expect(CommandRunnerMock, :run, fn _url, opts -> + assert opts == [:no_simulate, :dump_json, :custom_arg] + + {:ok, "{}"} + end) + + assert {:ok, _} = Video.download(@video_url, [:custom_arg]) + end + + test "it parses the output as JSON" do + expect(CommandRunnerMock, :run, fn _url, _opts -> {:ok, "{\"title\": \"Test\"}"} end) + + assert {:ok, %{"title" => "Test"}} = Video.download(@video_url) + end + + test "it directly passes along any errors" do + expect(CommandRunnerMock, :run, fn _url, _opts -> {:error, "Big issue", 1} end) + + assert {:error, "Big issue", 1} = Video.download(@video_url) + end + end +end diff --git a/test/pinchflat/downloader/video_downloader_test.exs b/test/pinchflat/downloader/video_downloader_test.exs new file mode 100644 index 0000000..36ad4a8 --- /dev/null +++ b/test/pinchflat/downloader/video_downloader_test.exs @@ -0,0 +1,35 @@ +defmodule Pinchflat.Downloader.VideoDownloaderTest do + use ExUnit.Case, async: true + import Mox + + alias Pinchflat.Profiles.MediaProfile + alias Pinchflat.Downloader.VideoDownloader + + @video_url "https://www.youtube.com/watch?v=TiZPUDkDYbk" + @media_profile %MediaProfile{ + output_path_template: "videos/{{ title }}.%(ext)s" + } + + setup :verify_on_exit! + + describe "download_for_media_profile/3" do + test "it calls the backend runner with the arguments built from the media profile" do + expect(CommandRunnerMock, :run, fn @video_url, opts -> + assert :no_simulate in opts + assert :dump_json in opts + assert {:output, "/tmp/yt-dlp/videos/%(title)S.%(ext)s"} in opts + + {:ok, "{}"} + end) + + assert {:ok, _} = VideoDownloader.download_for_media_profile(@video_url, @media_profile) + end + + test "it returns the parsed JSON output" do + expect(CommandRunnerMock, :run, fn _url, _opts -> {:ok, "{\"title\": \"Test\"}"} end) + + assert {:ok, %{"title" => "Test"}} = + VideoDownloader.download_for_media_profile(@video_url, @media_profile) + end + end +end diff --git a/test/pinchflat/profiles/options/yt_dlp/option_builder_test.exs b/test/pinchflat/profiles/options/yt_dlp/option_builder_test.exs new file mode 100644 index 0000000..eefb92d --- /dev/null +++ b/test/pinchflat/profiles/options/yt_dlp/option_builder_test.exs @@ -0,0 +1,18 @@ +defmodule Pinchflat.Profiles.Options.YtDlp.OptionBuilderTest do + use ExUnit.Case, async: true + + alias Pinchflat.Profiles.MediaProfile + alias Pinchflat.Profiles.Options.YtDlp.OptionBuilder + + @media_profile %MediaProfile{ + output_path_template: "videos/{{ title }}.%(ext)s" + } + + describe "build/1" do + test "it generates an expanded output path based on the given template" do + assert {:ok, res} = OptionBuilder.build(@media_profile) + + assert {:output, "/tmp/yt-dlp/videos/%(title)S.%(ext)s"} in res + end + end +end diff --git a/test/pinchflat/utils/string_utils_test.exs b/test/pinchflat/utils/string_utils_test.exs index 59ebbfc..60071fb 100644 --- a/test/pinchflat/utils/string_utils_test.exs +++ b/test/pinchflat/utils/string_utils_test.exs @@ -1,7 +1,7 @@ defmodule Pinchflat.Utils.StringUtilsTest do use ExUnit.Case, async: true - alias Pinchflat.Utils.StringUtils, as: StringUtils + alias Pinchflat.Utils.StringUtils describe "to_kebab_case/1" do test "converts a space-delimited string to kebab-case" do