[cli] Add tests for CSV delimiters and enforce constraints for delimiter lengths

Author: Breakthrough

scenedetect/_cli/__init__.py

@@ -1079,7 +1079,6 @@ def list_scenes_command(
         "quiet": ctx.config.get_value("list-scenes", "quiet", quiet) or ctx.quiet_mode,
         "row_separator": ctx.config.get_value("list-scenes", "row-separator"),
     }
-    # TODO(#423): Need to validate that col_separator is a 1-character string after decoding.
     ctx.add_command(cli_commands.list_scenes, list_scenes_args)
 
 

scenedetect/_cli/commands.py

@@ -98,8 +98,8 @@ def list_scenes(
                 scene_list=scenes,
                 include_cut_list=not skip_cuts,
                 cut_list=cuts,
-                col_separator=col_separator.encode("utf-8").decode("unicode_escape"),
-                row_separator=row_separator.encode("utf-8").decode("unicode_escape"),
+                col_separator=col_separator,
+                row_separator=row_separator,
             )
     # Suppress output if requested.
     if quiet:

scenedetect/_cli/config.py

@@ -60,6 +60,12 @@ def from_config(config_value: str, default: "ValidatedValue") -> "ValidatedValue
         """
         raise NotImplementedError()
 
+    def __repr__(self) -> str:
+        return str(self.value)
+
+    def __str__(self) -> str:
+        return str(self.value)
+
 
 class TimecodeValue(ValidatedValue):
     """Validator for timecode values in seconds (100.0), frames (100), or HH:MM:SS.
@@ -75,12 +81,6 @@ def __init__(self, value: Union[int, float, str]):
     def value(self) -> Union[int, float, str]:
         return self._value
 
-    def __repr__(self) -> str:
-        return str(self.value)
-
-    def __str__(self) -> str:
-        return str(self.value)
-
     @staticmethod
     def from_config(config_value: str, default: "TimecodeValue") -> "TimecodeValue":
         try:
@@ -121,12 +121,6 @@ def max_val(self) -> Union[int, float]:
         """Maximum value of the range."""
         return self._max_val
 
-    def __repr__(self) -> str:
-        return str(self.value)
-
-    def __str__(self) -> str:
-        return str(self.value)
-
     @staticmethod
     def from_config(config_value: str, default: "RangeValue") -> "RangeValue":
         try:
@@ -163,9 +157,6 @@ def __init__(self, value: Union[str, ContentDetector.Components]):
     def value(self) -> Tuple[float, float, float, float]:
         return self._value
 
-    def __repr__(self) -> str:
-        return str(self.value)
-
     def __str__(self) -> str:
         return "%.3f, %.3f, %.3f, %.3f" % self.value
 
@@ -199,9 +190,6 @@ def __init__(self, value: int):
     def value(self) -> int:
         return self._value
 
-    def __repr__(self) -> str:
-        return str(self.value)
-
     def __str__(self) -> str:
         if self.value is None:
             return "auto"
@@ -217,6 +205,42 @@ def from_config(config_value: str, default: "KernelSizeValue") -> "KernelSizeVal
             ) from ex
 
 
+class EscapedString(ValidatedValue):
+    """Strings that can contain escape sequences, e.g. the literal \n."""
+
+    def __init__(self, value: str, length_limit: int = 0):
+        self._value = value.encode("utf-8").decode("unicode_escape")
+        if length_limit and len(self._value) > length_limit:
+            raise OptionParseFailure(f"Value must be no longer than {length_limit} characters.")
+
+    @property
+    def value(self) -> str:
+        """Get the value after validation."""
+        return self._value
+
+    @staticmethod
+    def from_config(
+        config_value: str, default: "EscapedString", length_limit: int = 0
+    ) -> "EscapedString":
+        try:
+            return EscapedString(config_value, length_limit)
+        except (UnicodeDecodeError, UnicodeEncodeError) as ex:
+            raise OptionParseFailure(
+                "Value must be valid UTF-8 string with escape characters."
+            ) from ex
+
+
+class EscapedChar(EscapedString):
+    """Strings that can contain escape sequences but can be a maximum of 1 character in length."""
+
+    def __init__(self, value: str):
+        super().__init__(value, length_limit=1)
+
+    @staticmethod
+    def from_config(config_value: str, default: "EscapedString") -> "EscapedChar":
+        return EscapedString.from_config(config_value, default, length_limit=1)
+
+
 class TimecodeFormat(Enum):
     """Format to display timecodes."""
 
@@ -303,12 +327,12 @@ def format(self, timecode: FrameTimecode) -> str:
     },
     "list-scenes": {
         "cut-format": "timecode",
-        "col-separator": ",",
+        "col-separator": EscapedChar(","),
         "display-cuts": True,
         "display-scenes": True,
         "filename": "$VIDEO_NAME-Scenes.csv",
         "output": None,
-        "row-separator": "\n",
+        "row-separator": EscapedString("\n"),
         "no-output-file": False,
         "quiet": False,
         "skip-cuts": False,

scenedetect/_cli/context.py

@@ -215,7 +215,7 @@ def handle_options(
                 raise click.Abort()
 
         if self.config.config_dict:
-            logger.debug("Current configuration:\n%s", str(self.config.config_dict))
+            logger.debug("Current configuration:\n%s", str(self.config.config_dict).encode("utf-8"))
 
         logger.debug("Parsing program options.")
         if stats is not None and frame_skip:

scenedetect/scene_manager.py

@@ -218,7 +218,7 @@ def write_scene_list(
     cut_list: Optional[CutList] = None,
     col_separator: str = ",",
     row_separator: str = "\n",
-) -> None:
+):
     """Writes the given list of scenes to an output file handle in CSV format.
 
     Arguments:
@@ -229,8 +229,8 @@ def write_scene_list(
         cut_list: Optional list of FrameTimecode objects denoting the cut list (i.e. the frames
             in the video that need to be split to generate individual scenes). If not specified,
             the cut list is generated using the start times of each scene following the first one.
-        delimiter: Delimiter to use between values. Must be single character.
-        lineterminator: Line terminator to use between rows.
+        col_separator: Delimiter to use between values. Must be single character.
+        row_separator: Line terminator to use between rows.
 
     Raises:
         TypeError: "delimiter" must be a 1-character string

tests/test_cli.py

@@ -294,24 +294,90 @@ def test_cli_list_scenes(tmp_path: Path):
         )
         == 0
     )
-    # Add statsfile
+    output_path = tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv")
+    assert os.path.exists(output_path)
+    EXPECTED_CSV_OUTPUT = """Timecode List:,00:00:03.754
+Scene Number,Start Frame,Start Timecode,Start Time (seconds),End Frame,End Timecode,End Time (seconds),Length (frames),Length (timecode),Length (seconds)
+1,49,00:00:02.002,2.002,90,00:00:03.754,3.754,42,00:00:01.752,1.752
+2,91,00:00:03.754,3.754,144,00:00:06.006,6.006,54,00:00:02.252,2.252
+"""
+    assert output_path.read_text() == EXPECTED_CSV_OUTPUT
+
+
+def test_cli_list_scenes_skip_cuts(tmp_path: Path):
+    """Test `list-scenes` command with the -s/--skip-cuts option for RFC 4180 compliance."""
+    # Regular invocation
     assert (
         invoke_scenedetect(
-            "-i {VIDEO} -s {STATS} time {TIME} {DETECTOR} list-scenes",
+            "-i {VIDEO} time {TIME} {DETECTOR} list-scenes -s",
             output_dir=tmp_path,
         )
         == 0
     )
-    # Suppress output file
+    output_path = tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv")
+    assert os.path.exists(output_path)
+    EXPECTED_CSV_OUTPUT = """Scene Number,Start Frame,Start Timecode,Start Time (seconds),End Frame,End Timecode,End Time (seconds),Length (frames),Length (timecode),Length (seconds)
+1,49,00:00:02.002,2.002,90,00:00:03.754,3.754,42,00:00:01.752,1.752
+2,91,00:00:03.754,3.754,144,00:00:06.006,6.006,54,00:00:02.252,2.252
+"""
+    assert output_path.read_text() == EXPECTED_CSV_OUTPUT
+
+
+def test_cli_list_scenes_no_output(tmp_path: Path):
+    """Test `list-scenes` command with the -n flag."""
+    output_path = tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv")
     assert (
         invoke_scenedetect(
             "-i {VIDEO} time {TIME} {DETECTOR} list-scenes -n",
             output_dir=tmp_path,
         )
         == 0
     )
-    # TODO: Check for output files from regular invocation.
-    # TODO: Delete scene list and ensure is not recreated using -n.
+    assert not os.path.exists(output_path)
+
+
+def test_cli_list_scenes_custom_delimiter(tmp_path: Path):
+    """Test `list-scenes` command with custom delimiters set in a config file."""
+    config_path = tmp_path.joinpath("config.cfg")
+    config_path.write_text("""
+[list-scenes]
+col-separator = |
+row-separator = \\t
+""")
+    assert (
+        invoke_scenedetect(
+            f"-i {{VIDEO}} -c {config_path} time {{TIME}} {{DETECTOR}} list-scenes",
+            output_dir=tmp_path,
+        )
+        == 0
+    )
+    output_path = tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv")
+    assert os.path.exists(output_path)
+    EXPECTED_CSV_OUTPUT = """Timecode List:,00:00:03.754
+Scene Number,Start Frame,Start Timecode,Start Time (seconds),End Frame,End Timecode,End Time (seconds),Length (frames),Length (timecode),Length (seconds)
+1,49,00:00:02.002,2.002,90,00:00:03.754,3.754,42,00:00:01.752,1.752
+2,91,00:00:03.754,3.754,144,00:00:06.006,6.006,54,00:00:02.252,2.252
+"""
+    EXPECTED_CSV_OUTPUT = EXPECTED_CSV_OUTPUT.replace(",", "|").replace("\n", "\t")
+    assert output_path.read_text() == EXPECTED_CSV_OUTPUT
+
+
+def test_cli_list_scenes_rejects_multichar_col_separator(tmp_path: Path):
+    """Test `list-scenes` command with custom delimiters set in a config file."""
+    config_path = tmp_path.joinpath("config.cfg")
+    config_path.write_text("""
+[list-scenes]
+col-separator = ||
+""")
+    assert (
+        invoke_scenedetect(
+            f"-i {{VIDEO}} -c {config_path} time {{TIME}} {{DETECTOR}} list-scenes",
+            output_dir=tmp_path,
+        )
+        != 0
+    )
+    output_path = tmp_path.joinpath(f"{DEFAULT_VIDEO_NAME}-Scenes.csv")
+    assert not os.path.exists(output_path)
 
 
 @pytest.mark.skipif(condition=not is_ffmpeg_available(), reason="ffmpeg is not available")