Fix few --use-current-year bugs (#63)

Author: aostrowski

pre_commit_hooks/insert_license.py

@@ -32,6 +32,12 @@
 ])
 
 
+class LicenseUpdateError(Exception):
+    def __init__(self, message):
+        super().__init__(message)
+        self.message = message
+
+
 def main(argv=None):
     parser = argparse.ArgumentParser()
     parser.add_argument('filenames', nargs='*', help='filenames to check')
@@ -137,8 +143,9 @@ def process_files(args, changed_files, todo_files, license_info: LicenseInfo):
     :param changed_files: list of changed files
     :param todo_files: list of files where t.o.d.o. is detected
     :param license_info: license info named tuple
-    :return: True if some files were changed or t.o.d.o is detected
+    :return: True if some files were changed, t.o.d.o is detected or an error occurred while updating the year
     """
+    license_update_failed = False
     after_regex = args.insert_license_after_regex
     for src_filepath in args.filenames:
         src_file_content, encoding = _read_file_content(src_filepath)
@@ -168,14 +175,18 @@ def process_files(args, changed_files, todo_files, license_info: LicenseInfo):
                 fuzzy_ratio_cut_off=args.fuzzy_ratio_cut_off,
             )
         if license_header_index is not None:
-            if license_found(remove_header=args.remove_header,
-                             update_year_range=args.use_current_year,
-                             license_header_index=license_header_index,
-                             license_info=license_info,
-                             src_file_content=src_file_content,
-                             src_filepath=src_filepath,
-                             encoding=encoding):
-                changed_files.append(src_filepath)
+            try:
+                if license_found(remove_header=args.remove_header,
+                                 update_year_range=args.use_current_year,
+                                 license_header_index=license_header_index,
+                                 license_info=license_info,
+                                 src_file_content=src_file_content,
+                                 src_filepath=src_filepath,
+                                 encoding=encoding):
+                    changed_files.append(src_filepath)
+            except LicenseUpdateError as error:
+                print(error)
+                license_update_failed = True
         else:
             if fuzzy_match_header_index is not None:
                 if fuzzy_license_found(license_info=license_info,
@@ -194,7 +205,7 @@ def process_files(args, changed_files, todo_files, license_info: LicenseInfo):
                                      encoding=encoding,
                                      after_regex=after_regex):
                     changed_files.append(src_filepath)
-    return changed_files or todo_files
+    return changed_files or todo_files or license_update_failed
 
 
 def _read_file_content(src_filepath):
@@ -261,7 +272,9 @@ def license_not_found(  # pylint: disable=too-many-arguments
 
 def try_update_year_range(
     src_file_content: list[str],
+    src_filepath: str,
     license_header_index: int,
+    license_length: int
 ) -> tuple[Sequence[str], bool]:
     """
     Updates the years in a copyright header in src_file_content by
@@ -274,22 +287,25 @@ def try_update_year_range(
     :return: source file contents and a flag indicating update
     """
     current_year = datetime.now().year
-    for i in range(license_header_index, len(src_file_content)):
+    for i in range(license_header_index, license_header_index + license_length):
         line = src_file_content[i]
         matches = _YEAR_RANGE_PATTERN.findall(line)
         if matches:
             match = matches[-1]
             start_year = int(match[:4])
-            end_year = match[5:]
-            if not end_year or int(end_year) < current_year:
+            end_year = match[5:].lstrip(" -,")
+            if not end_year and start_year < current_year or end_year and int(end_year) < current_year:
                 updated = line.replace(match,
-                                    str(start_year) + '-' + str(current_year))
+                                       str(start_year) + '-' + str(current_year))
                 # verify the current list of years ends in the current one
                 if _YEARS_PATTERN.findall(updated)[-1][-4:] != str(current_year):
-                    print(f"Unable to update year range in line: {line.rstrip()}. Got: {updated.rstrip()}")
-                    break
+                    raise LicenseUpdateError(
+                        f"Year range detected in license header, but we were unable to update it.\n"
+                        f"File: {src_filepath}\nInput line: {line.rstrip()}\nDiscarded result: {updated.rstrip()}"
+                    )
                 src_file_content[i] = updated
                 return src_file_content, True
+            break
     return src_file_content, False
 
 
@@ -324,7 +340,7 @@ def license_found(
                                                 len(license_info.prefixed_license) + 1:]
         updated = True
     elif update_year_range:
-        src_file_content, updated = try_update_year_range(src_file_content, license_header_index)
+        src_file_content, updated = try_update_year_range(src_file_content, src_filepath, license_header_index, len(license_info.prefixed_license))
 
     if updated:
         with open(src_filepath, 'w', encoding=encoding) as src_file:

tests/insert_license_test.py

@@ -1,8 +1,10 @@
 from contextlib import contextmanager
 from datetime import datetime
 from itertools import product
+import io
 import os
 import shutil
+import sys
 import pytest
 
 from pre_commit_hooks.insert_license import main as insert_license, LicenseInfo
@@ -12,67 +14,81 @@
 
 
 @pytest.mark.parametrize(
-    ('license_file_path', 'src_file_path', 'comment_prefix', 'new_src_file_expected', 'fail_check', 'extra_args'),
-    map(lambda a: a[:1] + a[1], product(  # combine license files with other args
-        ('LICENSE_with_trailing_newline.txt', 'LICENSE_without_trailing_newline.txt'),
-        (
-            ('module_without_license.py', '#', 'module_with_license.py', True, None),
-            ('module_without_license_skip.py', '#', None, False, None),
-            ('module_with_license.py', '#', None, False, None),
-            ('module_with_license_todo.py', '#', None, True, None),
-
-            ('module_without_license.jinja', '{#||#}', 'module_with_license.jinja', True, None),
-            ('module_without_license_skip.jinja', '{#||#}', None, False, None),
-            ('module_with_license.jinja', '{#||#}', None, False, None),
-            ('module_with_license_todo.jinja', '{#||#}', None, True, None),
-
-            ('module_without_license_and_shebang.py', '#', 'module_with_license_and_shebang.py', True, None),
-            ('module_without_license_and_shebang_skip.py', '#', None, False, None),
-            ('module_with_license_and_shebang.py', '#', None, False, None),
-            ('module_with_license_and_shebang_todo.py', '#', None, True, None),
-
-            ('module_without_license.groovy', '//', 'module_with_license.groovy', True, None),
-            ('module_without_license_skip.groovy', '//', None, False, None),
-            ('module_with_license.groovy', '//', None, False, None),
-            ('module_with_license_todo.groovy', '//', None, True, None),
-
-            ('module_without_license.css', '/*| *| */', 'module_with_license.css', True, None),
-            ('module_without_license_and_few_words.css', '/*| *| */',
-                'module_with_license_and_few_words.css', True, None),  # Test fuzzy match does not match greedily
-            ('module_without_license_skip.css', '/*| *| */', None, False, None),
-            ('module_with_license.css', '/*| *| */', None, False, None),
-            ('module_with_license_todo.css', '/*| *| */', None, True, None),
-
-            ('main_without_license.cpp', '/*|\t| */', 'main_with_license.cpp', True, None),
-            ('main_iso8859_without_license.cpp', '/*|\t| */', 'main_iso8859_with_license.cpp', True, None),
-            ('module_without_license.txt', '', 'module_with_license_noprefix.txt', True, None),
-            ('module_without_license.py', '#', 'module_with_license_nospace.py', True, ['--no-space-in-comment-prefix']),
-            ('module_without_license.php', '/*| *| */', 'module_with_license.php', True, ['--insert-license-after-regex', '^<\\?php$']),
-            ('module_without_license.py', '#', 'module_with_license_noeol.py', True, ['--no-extra-eol']),
-
-            ('module_without_license.groovy', '//', 'module_with_license.groovy', True, ['--use-current-year']),
-            ('module_with_stale_year_in_license.py', '#', 'module_with_year_range_in_license.py', True, ['--use-current-year']),
-            ('module_with_stale_year_range_in_license.py', '#', 'module_with_year_range_in_license.py', True, ['--use-current-year']),
-            ('module_with_badly_formatted_stale_year_range_in_license.py', '#', 'module_with_badly_formatted_stale_year_range_in_license.py', False,
-             ['--use-current-year']),
+    ("license_file_path", "src_file_path", "comment_prefix", "new_src_file_expected", "message_expected", "fail_check", "extra_args"),
+    map(
+        lambda a: a[:1] + a[1],
+        product(  # combine license files with other args
+            ("LICENSE_with_trailing_newline.txt", "LICENSE_without_trailing_newline.txt"),
+            (
+                ("module_without_license.py", "#", "module_with_license.py", "", True, None),
+                ("module_without_license_skip.py", "#", None, "", False, None),
+                ("module_with_license.py", "#", None, "", False, None),
+                ("module_with_license_todo.py", "#", None, "", True, None),
+                ("module_without_license.jinja", "{#||#}", "module_with_license.jinja", "", True, None),
+                ("module_without_license_skip.jinja", "{#||#}", None, "", False, None),
+                ("module_with_license.jinja", "{#||#}", None, "", False, None),
+                ("module_with_license_todo.jinja", "{#||#}", None, "", True, None),
+                ("module_without_license_and_shebang.py", "#", "module_with_license_and_shebang.py", "", True, None),
+                ("module_without_license_and_shebang_skip.py", "#", None, "", False, None),
+                ("module_with_license_and_shebang.py", "#", None, "", False, None),
+                ("module_with_license_and_shebang_todo.py", "#", None, "", True, None),
+                ("module_without_license.groovy", "//", "module_with_license.groovy", "", True, None),
+                ("module_without_license_skip.groovy", "//", None, "", False, None),
+                ("module_with_license.groovy", "//", None, "", False, None),
+                ("module_with_license_todo.groovy", "//", None, "", True, None),
+                ("module_without_license.css", "/*| *| */", "module_with_license.css", "", True, None),
+                (
+                    "module_without_license_and_few_words.css",
+                    "/*| *| */",
+                    "module_with_license_and_few_words.css",
+                    "",
+                    True,
+                    None,
+                ),  # Test fuzzy match does not match greedily
+                ("module_without_license_skip.css", "/*| *| */", None, "", False, None),
+                ("module_with_license.css", "/*| *| */", None, "", False, None),
+                ("module_with_license_todo.css", "/*| *| */", None, "", True, None),
+                ("main_without_license.cpp", "/*|\t| */", "main_with_license.cpp", "", True, None),
+                ("main_iso8859_without_license.cpp", "/*|\t| */", "main_iso8859_with_license.cpp", "", True, None),
+                ("module_without_license.txt", "", "module_with_license_noprefix.txt", "", True, None),
+                ("module_without_license.py", "#", "module_with_license_nospace.py", "", True, ["--no-space-in-comment-prefix"]),
+                ("module_without_license.php", "/*| *| */", "module_with_license.php", "", True, ["--insert-license-after-regex", "^<\\?php$"]),
+                ("module_without_license.py", "#", "module_with_license_noeol.py", "", True, ["--no-extra-eol"]),
+                ("module_without_license.groovy", "//", "module_with_license.groovy", "", True, ["--use-current-year"]),
+                ("module_with_stale_year_in_license.py", "#", "module_with_year_range_in_license.py", "", True, ["--use-current-year"]),
+                ("module_with_stale_year_range_in_license.py", "#", "module_with_year_range_in_license.py", "", True, ["--use-current-year"]),
+                (
+                    "module_with_badly_formatted_stale_year_range_in_license.py",
+                    "#",
+                    "module_with_badly_formatted_stale_year_range_in_license.py",
+                    "module_with_badly_formatted_stale_year_range_in_license.py",
+                    True,
+                    ["--use-current-year"],
+                ),
+            ),
         ),
-    )),
+    ),
 )
 def test_insert_license(license_file_path,
                         src_file_path,
                         comment_prefix,
                         new_src_file_expected,
+                        message_expected,
                         fail_check,
                         extra_args,
                         tmpdir):
     encoding = 'ISO-8859-1' if 'iso8859' in src_file_path else 'utf-8'
     with chdir_to_test_resources():
-        path = tmpdir.join('src_file_path')
+        path = tmpdir.join(src_file_path)
         shutil.copy(src_file_path, path.strpath)
         args = ['--license-filepath', license_file_path, '--comment-style', comment_prefix, path.strpath]
         if extra_args is not None:
             args.extend(extra_args)
-        assert insert_license(args) == (1 if fail_check else 0)
+
+        with capture_stdout() as stdout:
+            assert insert_license(args) == (1 if fail_check else 0)
+            assert message_expected in stdout.getvalue()
+
         if new_src_file_expected:
             with open(new_src_file_expected, encoding=encoding) as expected_content_file:
                 expected_content = expected_content_file.read()
@@ -82,6 +98,33 @@ def test_insert_license(license_file_path,
             assert new_file_content == expected_content
 
 
+@pytest.mark.parametrize(
+    ('license_file_path', 'src_file_path', 'comment_prefix'),
+    map(lambda a: a[:1] + a[1], product(  # combine license files with other args
+        ('LICENSE_with_trailing_newline.txt', 'LICENSE_without_trailing_newline.txt'),
+        (
+            ('module_with_license.groovy', '//'),
+            ('module_with_license_and_numbers.py', '#'),
+            ('module_with_year_range_in_license.py', '#'),
+            ('module_with_spaced_year_range_in_license.py', '#'),
+        ),
+    )),
+)
+def test_insert_license_current_year_already_there(license_file_path,
+                                                   src_file_path,
+                                                   comment_prefix,
+                                                   tmpdir):
+    with chdir_to_test_resources():
+        with open(src_file_path, encoding="utf-8") as src_file:
+            input_contents = src_file.read().replace("2017", str(datetime.now().year))
+        path = tmpdir.join('src_file_path')
+        with open(path.strpath, "w", encoding="utf-8") as input_file:
+            input_file.write(input_contents)
+
+        args = ['--license-filepath', license_file_path, '--comment-style', comment_prefix, "--use-current-year", path.strpath]
+        assert insert_license(args) == 0
+
+
 @pytest.mark.parametrize(
     ('license_file_path', 'src_file_path', 'comment_style', 'new_src_file_expected', 'fail_check'),
     map(lambda a: a[:1] + a[1], product(  # combine license files with other args
@@ -229,8 +272,18 @@ def test_remove_license(license_file_path,
 def chdir_to_test_resources():
     prev_dir = os.getcwd()
     try:
-        res_dir = os.path.dirname(os.path.realpath(__file__)) +'/resources'
+        res_dir = os.path.dirname(os.path.realpath(__file__)) + '/resources'
         os.chdir(res_dir)
         yield
     finally:
         os.chdir(prev_dir)
+
+
+@contextmanager
+def capture_stdout():
+    try:
+        captured = io.StringIO()
+        sys.stdout = captured
+        yield captured
+    finally:
+        sys.stdout = sys.__stdout__

tests/resources/module_with_license_and_numbers.py

@@ -0,0 +1,6 @@
+# Copyright (C) 2016-2017 Teela O'Malley
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+
+import sys
+sys.stdout.write("1985")  # 1985

tests/resources/module_with_spaced_year_range_in_license.py

@@ -0,0 +1,6 @@
+# Copyright (C) 2015 - 2017 Teela O'Malley
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+
+import sys
+sys.stdout.write("FOO")