Repository: Diaoul/subliminal (python)

Issue:
Add an option to save log to file
The current solution (on Linux) to keep logs of subliminal outputs is with:

```bash
subliminal --debug download -l en video.mkv > subliminal.log
```
It would be better (and it would work cross-platform) to add an option to the CLI.


Pull request:
Add a CLI option --logfile to log to file
closes #1223 

The level can be specified with `--logfile-level`, defaults to 'DEBUG'.

Add a `scan_path` function to simplify the code for the CLI and add a log message in case of a GuessingError, otherwise it was only shown in the output with `verbose > 0`.

Patch:
diff --git a/changelog.d/1223.change.rst b/changelog.d/1223.change.rst
new file mode 100644
index 0000000..ba5cb0e
--- /dev/null
+++ b/changelog.d/1223.change.rst
@@ -0,0 +1,1 @@
+add a CLI option --logfile to log to file. Level can be specified with --logfile-level, default to DEBUG
diff --git a/changelog.d/891.change.rst b/changelog.d/891.change.rst
new file mode 100644
index 0000000..3977acc
--- /dev/null
+++ b/changelog.d/891.change.rst
@@ -0,0 +1,3 @@
+add CLI options --force-embedded-subtitles and --force-external-subtitles.
+They are fine-tuned --force options to ignore only embedded or external existing subtitles.
+They are superseded by --force.
diff --git a/src/subliminal/cli.py b/src/subliminal/cli.py
index 2a8f75a..185a88b 100644
--- a/src/subliminal/cli.py
+++ b/src/subliminal/cli.py
@@ -3,7 +3,6 @@
 
 from __future__ import annotations
 
-import glob
 import logging
 import os
 import pathlib
@@ -11,6 +10,7 @@ import re
 from collections import defaultdict
 from collections.abc import Mapping
 from datetime import timedelta
+from pathlib import Path
 from typing import TYPE_CHECKING, Any
 
 import click
@@ -40,8 +40,7 @@ from subliminal import (
 from subliminal.core import (
     ARCHIVE_EXTENSIONS,
     collect_video_filepaths,
-    scan_name,
-    scan_video_or_archive,
+    scan_path,
     search_external_subtitles,
 )
 from subliminal.exceptions import GuessingError
@@ -297,6 +296,21 @@ options_from_refiners = options_from_managers('refiner', refiner_options, group=
 @options_from_providers
 @options_from_refiners
 @click.option('--debug', is_flag=True, help='Print useful information for debugging subliminal and for reporting bugs.')
+@click.option(
+    '--logfile',
+    type=str,
+    default='',
+    help=(
+        'If defined, record information to the specified log file. '
+        'If the file already exists, new logs are appended to the file.'
+    ),
+)
+@click.option(
+    '--logfile-level',
+    type=click.Choice(['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']),
+    default='DEBUG',
+    help='The logging level used for the log file.',
+)
 @click.version_option(__version__)
 @click.pass_context
 def subliminal(
@@ -304,30 +318,58 @@ def subliminal(
     /,
     cache_dir: str,
     debug: bool,
+    logfile: os.PathLike[str] | None,
+    logfile_level: str,
     **kwargs: Any,
 ) -> None:
     """Subtitles, faster than your thoughts."""
-    cache_dir = os.path.expanduser(cache_dir)
+    # make sure to expand user
+    cache_dir_path = Path(cache_dir).expanduser()
     # create cache directory
     try:
-        os.makedirs(cache_dir)
+        cache_dir_path.mkdir(parents=True)
     except OSError:
-        if not os.path.isdir(cache_dir):
+        if not cache_dir_path.is_dir():
             raise
 
     # configure cache
     region.configure(
         'dogpile.cache.dbm',
         expiration_time=timedelta(days=30),
-        arguments={'filename': os.path.join(cache_dir, cache_file), 'lock_factory': MutexLock},
+        arguments={'filename': os.fspath(cache_dir_path / cache_file), 'lock_factory': MutexLock},
     )
 
+    # Set the logger level to DEBUG in case debug or logfile is defined
+    subliminal_logger = logging.getLogger('subliminal')
+    subliminal_logger.setLevel(logging.DEBUG)
+
+    # configure logging file
+    if logfile:
+        # make sure to expand user
+        logfile_path = Path(logfile).expanduser()
+        try:
+            # make sure the parent directories exist
+            logfile_path.parent.mkdir(parents=True)
+        except OSError:
+            if not logfile_path.parent.is_dir():
+                raise
+
+        file_handler = logging.FileHandler(logfile_path)
+        file_handler.setFormatter(
+            logging.Formatter(
+                '%(asctime)s | %(levelname)-8s | %(name)s | %(message)s',
+                datefmt='%Y-%m-%d %H:%M:%S',
+            ),
+        )
+        file_handler.setLevel(logfile_level)
+        subliminal_logger.addHandler(file_handler)
+
     # configure logging
     if debug:
         handler = logging.StreamHandler()
         handler.setFormatter(logging.Formatter(logging.BASIC_FORMAT))
-        logging.getLogger('subliminal').addHandler(handler)
-        logging.getLogger('subliminal').setLevel(logging.DEBUG)
+        handler.setLevel(logging.DEBUG)
+        subliminal_logger.addHandler(handler)
         # log about the config file
         msg = ctx.obj['debug_message']
         logger.info(msg)
@@ -364,8 +406,9 @@ def subliminal(
 def cache(ctx: click.Context, clear_subliminal: bool) -> None:
     """Cache management."""
     if clear_subliminal and ctx.parent and 'cache_dir' in ctx.parent.params:
-        for file in glob.glob(os.path.join(ctx.parent.params['cache_dir'], cache_file) + '*'):
-            os.remove(file)
+        cache_dir_path = Path(ctx.parent.params['cache_dir'])
+        for file in (cache_dir_path / cache_file).glob('*'):
+            file.unlink()
         click.echo("Subliminal's cache cleared.")
     else:
         click.echo('Nothing done.')
@@ -380,7 +423,13 @@ def cache(ctx: click.Context, clear_subliminal: bool) -> None:
     multiple=True,
     help='Language as IETF code, e.g. en, pt-BR (can be used multiple times).',
 )
-@click.option('-p', '--provider', type=PROVIDER, multiple=True, help='Provider to use (can be used multiple times).')
+@click.option(
+    '-p',
+    '--provider',
+    type=PROVIDER,
+    multiple=True,
+    help='Provider to use (can be used multiple times).',
+)
 @click.option(
     '-pp',
     '--extend-provider',
@@ -401,7 +450,13 @@ def cache(ctx: click.Context, clear_subliminal: bool) -> None:
         'Supersedes the providers used or ignored in the configuration file.'
     ),
 )
-@click.option('-r', '--refiner', type=REFINER, multiple=True, help='Refiner to use (can be used multiple times).')
+@click.option(
+    '-r',
+    '--refiner',
+    type=REFINER,
+    multiple=True,
+    help='Refiner to use (can be used multiple times).',
+)
 @click.option(
     '-rr',
     '--extend-refiner',
@@ -429,7 +484,12 @@ def cache(ctx: click.Context, clear_subliminal: bool) -> None:
     multiple=True,
     help='Subtitle ids to ignore (can be used multiple times).',
 )
-@click.option('-a', '--age', type=AGE, help='Filter videos newer than AGE, e.g. 12h, 1w2d.')
+@click.option(
+    '-a',
+    '--age',
+    type=AGE,
+    help='Filter videos newer than AGE, e.g. 12h, 1w2d.',
+)
 @click.option(
     '--use_creation_time',
     'use_ctime',
@@ -474,12 +534,27 @@ def cache(ctx: click.Context, clear_subliminal: bool) -> None:
         'Do not use this unless your media player requires it.'
     ),
 )
+@click.option(
+    '--force-external-subtitles',
+    is_flag=True,
+    default=False,
+    help='Force download even if an external subtitle already exists. Superseded by `--force`.',
+)
+@click.option(
+    '--force-embedded-subtitles',
+    is_flag=True,
+    default=False,
+    help='Force download even if an embedded subtitle already exists. Superseded by `--force`.',
+)
 @click.option(
     '-f',
     '--force',
     is_flag=True,
     default=False,
-    help='Force download even if a subtitle already exist.',
+    help=(
+        'Force download even if a subtitle already exists. '
+        'Supersedes `--force-external-subtitles` and `--force-embedded-subtitles`.'
+    ),
 )
 @click.option(
     '-W',
@@ -585,6 +660,8 @@ def download(
     encoding: str | None,
     subtitle_format: str | None,
     single: bool,
+    force_external_subtitles: bool,
+    force_embedded_subtitles: bool,
     force: bool,
     skip_wrong_fps: bool,
     hearing_impaired: tuple[bool | None, ...],
@@ -621,6 +698,8 @@ def download(
     if len(foreign_only) > 0:
         foreign_only_flag = foreign_only[-1]
 
+    logger.info('Download with subliminal version %s', __version__)
+    # Make sure verbose is maximal if debug is specified to show ALL the messages
     debug = obj.get('debug', False)
     if debug:
         verbose = 3
@@ -675,14 +754,16 @@ def download(
             video_candidates: list[Video] = []
             for filepath in collected_filepaths:
                 exists = os.path.exists(p)
+                filepath_or_name = f'{filepath} ({name})' if name else filepath
                 try:
-                    # existing path
-                    if exists:  # noqa: SIM108
-                        video = scan_video_or_archive(filepath, name=name)
-                    else:
-                        video = scan_name(filepath, name=name)
+                    video = scan_path(filepath, name=name)
 
                 except GuessingError as e:
+                    logger.exception(
+                        'Cannot guess information about %s %s',
+                        'path' if exists else 'non-existing path',
+                        filepath_or_name,
+                    )
                     # Show a simple error message
                     if verbose > 0:
                         # new line was already added with debug
@@ -696,7 +777,7 @@ def download(
                     logger.exception(
                         'Unexpected error while collecting %s %s',
                         'path' if exists else 'non-existing path',
-                        f'{filepath} ({name})' if name else filepath,
+                        filepath_or_name,
                     )
                     errored_paths.append(filepath)
                     continue
@@ -705,14 +786,14 @@ def download(
 
             # check and refine videos
             for video in video_candidates:
-                if not force:
+                if not force and not force_external_subtitles:
                     video.subtitles |= set(search_external_subtitles(video.name, directory=directory).values())
                 if check_video(video, languages=language_set, age=age, use_ctime=use_ctime, undefined=single):
                     refine(
                         video,
                         refiners=use_refiners,
                         refiner_configs=obj['refiner_configs'],
-                        embedded_subtitles=not force,
+                        embedded_subtitles=not force and not force_embedded_subtitles,
                         providers=use_providers,
                         languages=language_set,
                     )
diff --git a/src/subliminal/core.py b/src/subliminal/core.py
index adc350f..2417457 100644
--- a/src/subliminal/core.py
+++ b/src/subliminal/core.py
@@ -14,7 +14,7 @@ from babelfish import Language, LanguageReverseError  # type: ignore[import-unty
 from guessit import guessit  # type: ignore[import-untyped]
 
 from .archives import ARCHIVE_ERRORS, ARCHIVE_EXTENSIONS, is_supported_archive, scan_archive
-from .exceptions import ArchiveError
+from .exceptions import ArchiveError, DiscardingError
 from .extensions import (
     discarded_episode_refiners,
     discarded_movie_refiners,
@@ -143,12 +143,17 @@ class ProviderPool:
         # list subtitles
         logger.info('Listing subtitles with provider %r and languages %r', provider, provider_languages)
         try:
-            return self[provider].list_subtitles(video, provider_languages)
-        except Exception as e:  # noqa: BLE001
+            subtitles = self[provider].list_subtitles(video, provider_languages)
+        except DiscardingError as e:
+            handle_exception(e, f'Provider {provider}')
+            # return None to discard this provider with a known error
+            return None
+        except Exception as e:  # noqa: BLE001  # pragma: no cover
             handle_exception(e, f'Provider {provider}')
+            # return [] so the provider is not discarded with unknown error
+            return []
 
-        # return None to discard this provider
-        return None
+        return subtitles
 
     def list_subtitles(self, video: Video, languages: Set[Language]) -> list[Subtitle]:
         """List subtitles.
@@ -548,6 +553,20 @@ def scan_video_or_archive(path: str | os.PathLike, name: str | None = None) -> V
     raise ValueError(msg)  # pragma: no cover
 
 
+def scan_path(filepath: str | os.PathLike[str], *, name: str | None = None) -> Video:
+    """Scan a video or an archive from a `path`, maybe non-existing.
+
+    :param str path: path to the video or archive, may be an existing path or not.
+    :param str name: if defined, name to use with guessit instead of the path.
+    :return: the scanned video.
+    :rtype: :class:`~subliminal.video.Video`
+    :raises: :class:`ValueError`: video path is not well defined.
+    """
+    if not os.path.isfile(filepath):
+        return scan_name(filepath, name=name)
+    return scan_video_or_archive(filepath, name=name)
+
+
 def collect_video_filepaths(
     path: str | os.PathLike,
     *,
diff --git a/src/subliminal/exceptions.py b/src/subliminal/exceptions.py
index 85bab29..78e08a0 100644
--- a/src/subliminal/exceptions.py
+++ b/src/subliminal/exceptions.py
@@ -31,25 +31,31 @@ class NotInitializedProviderError(ProviderError):
     pass
 
 
-class ConfigurationError(ProviderError):
+class DiscardingError(ProviderError):
+    """Exception raised by providers that should lead to discard this provider."""
+
+    pass
+
+
+class ConfigurationError(DiscardingError):
     """Exception raised by providers when badly configured."""
 
     pass
 
 
-class AuthenticationError(ProviderError):
+class AuthenticationError(DiscardingError):
     """Exception raised by providers when authentication failed."""
 
     pass
 
 
-class ServiceUnavailable(ProviderError):
+class ServiceUnavailable(DiscardingError):
     """Exception raised when status is '503 Service Unavailable'."""
 
     pass
 
 
-class DownloadLimitExceeded(ProviderError):
+class DownloadLimitExceeded(DiscardingError):
     """Exception raised by providers when download limit is exceeded."""
 
     pass
diff --git a/src/subliminal/providers/mock.py b/src/subliminal/providers/mock.py
index fcebd93..f1cda68 100644
--- a/src/subliminal/providers/mock.py
+++ b/src/subliminal/providers/mock.py
@@ -10,7 +10,7 @@ from typing import TYPE_CHECKING, Any, ClassVar
 from babelfish import LANGUAGES, Language  # type: ignore[import-untyped]
 from guessit import guessit  # type: ignore[import-untyped]
 
-from subliminal.exceptions import NotInitializedProviderError, ProviderError
+from subliminal.exceptions import DiscardingError, NotInitializedProviderError
 from subliminal.matches import guess_matches
 from subliminal.subtitle import Subtitle
 from subliminal.video import Episode, Movie, Video
@@ -130,7 +130,7 @@ class MockProvider(Provider):
         """Query the provider for subtitles."""
         if self.is_broken:
             msg = f'Mock provider {self.__class__.__name__} query raised an error'
-            raise ProviderError(msg)
+            raise DiscardingError(msg)
 
         subtitles = []
         for lang in languages:
@@ -149,7 +149,7 @@ class MockProvider(Provider):
         """List all the subtitles for the video."""
         if self.is_broken:
             msg = f'Mock provider {self.__class__.__name__} list_subtitles raised an error'
-            raise ProviderError(msg)
+            raise DiscardingError(msg)
 
         subtitles = [
             subtitle
@@ -169,7 +169,7 @@ class MockProvider(Provider):
         """Download the content of the subtitle."""
         if self.is_broken:
             msg = f'Mock provider {self.__class__.__name__} download_subtitle raised an error'
-            raise ProviderError(msg)
+            raise DiscardingError(msg)
 
         logger.info(
             'Mock provider %s download subtitle %s',


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index 9d8ebdf..0ab1029 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -16,6 +16,7 @@ from subliminal.core import (
     save_subtitles,
     scan_archive,
     scan_name,
+    scan_path,
     scan_video,
     scan_video_or_archive,
     scan_videos,
@@ -339,15 +340,36 @@ def test_scan_archive_invalid_extension(
     monkeypatch.chdir(tmp_path)
     movie_name = os.path.splitext(movies['interstellar'].name)[0] + '.mp3'
     ensure(tmp_path / movie_name)
-    with pytest.raises(ArchiveError) as excinfo:
+    with pytest.raises(ArchiveError, match="'.mp3' is not a valid archive"):
         scan_archive(movie_name)
-    assert str(excinfo.value) == "'.mp3' is not a valid archive"
+
+
+def test_scan_path(
+    movies: dict[str, Movie],
+    tmp_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    import subliminal
+
+    mock = Mock()
+    monkeypatch.setattr(subliminal.core, 'scan_video_or_archive', mock)
+
+    video = movies['interstellar']
+    ensure(tmp_path / video.name)
+    monkeypatch.chdir(tmp_path)
+
+    # Non-existing path
+    scan_path(f'non-existing-{video.name}')
+    mock.assert_not_called()
+
+    # Existing path
+    scan_path(video.name)
+    mock.assert_called_once()
 
 
 def test_scan_videos_path_does_not_exist(movies: dict[str, Movie]) -> None:
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ValueError, match='Path does not exist'):
         scan_videos(movies['man_of_steel'].name)
-    assert str(excinfo.value) == 'Path does not exist'
 
 
 def test_scan_videos_path_is_not_a_directory(
@@ -358,9 +380,8 @@ def test_scan_videos_path_is_not_a_directory(
     video = movies['man_of_steel']
     monkeypatch.chdir(tmp_path)
     ensure(tmp_path / video.name)
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ValueError, match='Path is not a directory'):
         scan_videos(movies['man_of_steel'].name)
-    assert str(excinfo.value) == 'Path is not a directory'
 
 
 def test_scan_videos(movies: dict[str, Movie], tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: