Merge pull request #854 from carmenbianca/exit-if-unrecognised

Re-introduce old behaviour under --skip-if-unrecognised

CHANGELOG.md

@@ -59,7 +59,8 @@ CLI command and its behaviour. There are no guarantees of stability for the
   or old setups", which "have the submodule's git directory inside the submodule
   instead of embedded into the superproject's git directory". (#687)
 - When running `annotate` on a file with an unrecognised file path,
-  automatically create a `.license` file. (#851)
+  automatically create a `.license` file. The old behaviour of exiting on
+  unrecognised paths is now behind `--exit-if-unrecognised`. (#851, #853)
 - No longer scan binary or uncommentable files for their contents in search of
   REUSE information. (#825)
 - `--force-dot-license` and `--skip-unrecognised` are now mutually exclusive on
@@ -80,6 +81,8 @@ CLI command and its behaviour. There are no guarantees of stability for the
 ### Fixed
 
 - Reduced python-debian minimum version to 0.1.34. (#808)
+- Fix issue in `annotate` where `--single-line` and `--multi-line` would not
+  correctly raise an error with an incompatible comment style. (#853)
 
 ### Security
 

src/reuse/_annotate.py

@@ -54,27 +54,27 @@
 
 
 def verify_paths_line_handling(
+    args: Namespace,
     paths: Iterable[Path],
-    parser: ArgumentParser,
-    force_single: bool,
-    force_multi: bool,
 ) -> None:
-    """This function aborts the parser when *force_single* or *force_multi* is
+    """This function aborts the parser when --single-line or --multi-line is
     used, but the file type does not support that type of comment style.
     """
     for path in paths:
-        style = _get_comment_style(path)
+        style = NAME_STYLE_MAP.get(args.style)
+        if style is None:
+            style = _get_comment_style(path)
         if style is None:
             continue
-        if force_single and not style.can_handle_single():
-            parser.error(
+        if args.single_line and not style.can_handle_single():
+            args.parser.error(
                 _(
                     "'{path}' does not support single-line comments, please"
                     " do not use --single-line"
                 ).format(path=path)
             )
-        if force_multi and not style.can_handle_multi():
-            parser.error(
+        if args.multi_line and not style.can_handle_multi():
+            args.parser.error(
                 _(
                     "'{path}' does not support multi-line comments, please"
                     " do not use --multi-line"
@@ -123,9 +123,10 @@ def add_header_to_file(
     """Helper function."""
     # pylint: disable=too-many-arguments,too-many-locals
     result = 0
-    if style is not None:
-        comment_style: Optional[Type[CommentStyle]] = NAME_STYLE_MAP.get(style)
-    else:
+    comment_style: Optional[Type[CommentStyle]] = NAME_STYLE_MAP.get(
+        cast(str, style)
+    )
+    if comment_style is None:
         comment_style = _get_comment_style(path)
     if comment_style is None:
         if skip_unrecognised:
@@ -336,6 +337,30 @@ def verify_write_access(
         )
 
 
+def verify_paths_comment_style(args: Namespace, paths: Iterable[Path]) -> None:
+    """Exit if --exit-if-unrecognised is enabled and one of the paths has an
+    unrecognised style.
+    """
+    if args.exit_if_unrecognised:
+        unrecognised_files = []
+
+        for path in paths:
+            if not _has_style(path):
+                unrecognised_files.append(path)
+
+        if unrecognised_files:
+            args.parser.error(
+                "{}\n\n{}".format(
+                    _(
+                        "The following files do not have a recognised file"
+                        " extension. Please use --style, --force-dot-license or"
+                        " --skip-unrecognised:"
+                    ),
+                    "\n".join(str(path) for path in unrecognised_files),
+                )
+            )
+
+
 def add_arguments(parser: ArgumentParser) -> None:
     """Add arguments to parser."""
     parser.add_argument(
@@ -408,8 +433,8 @@ def add_arguments(parser: ArgumentParser) -> None:
         action="store_true",
         help=_("force multi-line comment style, optional"),
     )
-    skip_force_mutex_group = parser.add_mutually_exclusive_group()
-    skip_force_mutex_group.add_argument(
+    style_mutex_group = parser.add_mutually_exclusive_group()
+    style_mutex_group.add_argument(
         "--force-dot-license",
         action="store_true",
         help=_("write a .license file instead of a header inside the file"),
@@ -429,7 +454,14 @@ def add_arguments(parser: ArgumentParser) -> None:
             "do not replace the first header in the file; just add a new one"
         ),
     )
-    skip_force_mutex_group.add_argument(
+    style_mutex_group.add_argument(
+        "--exit-if-unrecognised",
+        action="store_true",
+        help=_(
+            "exit prematurely if one of the files has no defined comment style"
+        ),
+    )
+    style_mutex_group.add_argument(
         "--skip-unrecognised",
         action="store_true",
         help=_("skip files with unrecognised comment styles"),
@@ -450,17 +482,13 @@ def run(args: Namespace, project: Project, out: IO[str] = sys.stdout) -> int:
 
     paths = all_paths(args, project)
 
+    verify_paths_comment_style(args, paths)
+
     if not args.force_dot_license:
         verify_write_access(paths, args.parser)
 
     # Verify line handling and comment styles before proceeding
-    if args.style is None and not args.force_dot_license:
-        verify_paths_line_handling(
-            paths,
-            args.parser,
-            force_single=args.single_line,
-            force_multi=args.multi_line,
-        )
+    verify_paths_line_handling(args, paths)
 
     template, commented = get_template(args, project)
 

tests/test_main_annotate.py

@@ -1072,8 +1072,7 @@ def test_annotate_skip_force_mutually_exclusive(fake_repository):
         )
 
 
-@pytest.mark.parametrize("skip_option", [("--skip-unrecognised"), ("")])
-def test_annotate_multi_line_not_supported(fake_repository, skip_option):
+def test_annotate_multi_line_not_supported(fake_repository):
     """Expect a fail if --multi-line is not supported for a file type."""
     with pytest.raises(SystemExit):
         main(
@@ -1084,14 +1083,37 @@ def test_annotate_multi_line_not_supported(fake_repository, skip_option):
                 "--copyright",
                 "Jane Doe",
                 "--multi-line",
-                skip_option,
                 "src/source_code.py",
             ]
         )
 
 
-@pytest.mark.parametrize("skip_option", [("--skip-unrecognised"), ("")])
-def test_annotate_single_line_not_supported(fake_repository, skip_option):
+def test_annotate_multi_line_not_supported_custom_style(
+    fake_repository, capsys
+):
+    """--multi-line also fails when used with a style that doesn't support it
+    through --style.
+    """
+    (fake_repository / "foo.foo").write_text("foo")
+    with pytest.raises(SystemExit):
+        main(
+            [
+                "annotate",
+                "--license",
+                "GPL-3.0-or-later",
+                "--copyright",
+                "Jane Doe",
+                "--multi-line",
+                "--style",
+                "python",
+                "foo.foo",
+            ],
+        )
+
+    assert "'foo.foo' does not support multi-line" in capsys.readouterr().err
+
+
+def test_annotate_single_line_not_supported(fake_repository):
     """Expect a fail if --single-line is not supported for a file type."""
     with pytest.raises(SystemExit):
         main(
@@ -1102,7 +1124,6 @@ def test_annotate_single_line_not_supported(fake_repository, skip_option):
                 "--copyright",
                 "Jane Doe",
                 "--single-line",
-                skip_option,
                 "src/source_code.html",
             ]
         )
@@ -1290,4 +1311,31 @@ def test_annotate_recursive_on_file(fake_repository, stringio, mock_date_today):
     assert result == 0
 
 
+def test_annotate_exit_if_unrecognised(
+    fake_repository, stringio, mock_date_today
+):
+    """Expect error and no edited files if at least one file has not been
+    recognised, with --exit-if-unrecognised enabled."""
+    (fake_repository / "baz").mkdir(parents=True)
+    (fake_repository / "baz/foo.py").write_text("foo")
+    (fake_repository / "baz/bar.unknown").write_text("bar")
+    (fake_repository / "baz/baz.sh").write_text("baz")
+
+    with pytest.raises(SystemExit):
+        main(
+            [
+                "annotate",
+                "--license",
+                "Apache-2.0",
+                "--copyright",
+                "Jane Doe",
+                "--recursive",
+                "--exit-if-unrecognised",
+                "baz/",
+            ]
+        )
+
+    assert "Jane Doe" not in (fake_repository / "baz/foo.py").read_text()
+
+
 # REUSE-IgnoreEnd