fix redirect error reporting

tests/test_articles/wiki/redirect.yaml

@@ -1 +1,4 @@
-"redirected_article": "asdf"
+redirected_article: asdf
+
+hi_this_is_a_redirect: redirected_sections#redirected-heading-that-exists
+hi_this_is_a_bad_redirect: redirected_sections#redirected-heading-that-does-not-exist

tests/test_articles/wiki/redirected_sections/en.md

@@ -0,0 +1,6 @@
+## Redirected heading that exists
+
+- [overwritten section](/wiki/hi_this_is_a_redirect#fragment-that-should-be-overwritten-by-redirect) (ok)
+- [no section](/wiki/hi_this_is_a_redirect) (ok)
+- [redirected bad section](/wiki/hi_this_is_a_bad_redirect) (not ok)
+- [bad redirected section but good link](/wiki/hi_this_is_a_bad_redirect#redirected-heading-that-exists) (not ok)

tests/test_link_checker.py

@@ -833,7 +833,6 @@ def test__valid_redirect(self, root, payload):
     @pytest.mark.parametrize(
         "payload",
         [
-            {"link": "/wiki/Old_location#totally-wrong-heading", "redirect": '"old_location": "Target_article"'},
             {"link": "/wiki/Old_location", "redirect": '"old_location": "Target_article#totally-wrong-heading"'},
             {"link": "/wiki/Old_location#some-old-heading", "redirect": '"old_location": "Target_article#totally-wrong-heading"'}, # redirected section takes priority
         ]
@@ -860,6 +859,44 @@ def test__invalid_redirect(self, root, payload):
         }
         redirects = redirect_parser.load_redirects('wiki/redirect.yaml')
 
+        link = link_parser.find_link(f"Please follow the [target article]({payload['link']}).")
+        assert link
+        error = link_checker.check_link(
+            article=dummy_article('wiki/New_article/en.md'), link=link, redirects=redirects, references={}, all_articles=all_articles
+        )
+        assert isinstance(error, error_types.BrokenRedirectIdentifierError)
+        assert error.identifier == 'totally-wrong-heading'
+        assert error.link.parsed_location.fragment == (payload["link"].split("#")[1] if "#" in payload["link"] else "")
+        assert error.path == 'wiki/Target_article/en.md'
+
+    @pytest.mark.parametrize(
+        "payload",
+        [
+            {"link": "/wiki/Old_location#totally-wrong-heading", "redirect": '"old_location": "Target_article"'},
+        ]
+    )
+    def test__invalid_section_link_with_redirect(self, root, payload):
+        utils.create_files(
+            root,
+            ('wiki/redirect.yaml', payload['redirect']),
+            ('wiki/New_article/en.md', '# New article'),
+            (
+                'wiki/Target_article/en.md',
+                textwrap.dedent('''
+                    # Included article
+
+                    ## Subheading
+
+                    This line exists.
+                ''').strip()
+            )
+        )
+        all_articles = {
+            path: article_parser.parse(path)
+            for path in ('wiki/New_article/en.md', 'wiki/Target_article/en.md')
+        }
+        redirects = redirect_parser.load_redirects('wiki/redirect.yaml')
+
         link = link_parser.find_link(f"Please follow the [target article]({payload['link']}).")
         assert link
         error = link_checker.check_link(

tests/visual/test_check_links.py

@@ -9,7 +9,7 @@
     cases=[
         VisualTestCase(
             name="malformed_link",
-            description="Malformed link (1 error)",
+            description="Malformed link (2 errors)",
             function=lambda : check_links.main("--root", "tests/test_articles", "--target", "wiki/malformed_link/en.md")
         ),
         VisualTestCase(
@@ -37,5 +37,10 @@
             description="Missing identifier (3 errors)",
             function=lambda : check_links.main("--root", "tests/test_articles",  "--target", "wiki/missing_identifier/en.md", "news/2023/news-post-bad-section-link.md")
         ),
+        VisualTestCase(
+            name="redirected_section_links",
+            description="Redirected section links (2 errors)",
+            function=lambda : check_links.main("--root", "tests/test_articles",  "--target", "wiki/redirected_sections/en.md")
+        ),
     ]
 )

wikitools/errors.py

@@ -9,7 +9,7 @@ class LinkError:
     link: link_parser.Link
 
     def pretty(self):
-        return f'{console.blue("Note:")} {repr(self)}'
+        return f'{console.blue("Note:")} ' + repr(self).replace("\n", "\n      ")
 
     def pretty_location(self, article_path, lineno):
         return "{}: {}".format(
@@ -74,9 +74,13 @@ class BrokenRedirectError(
     redirect_lineno: int
     redirect_destination: str
 
+    _colourise_fragment_only_in_redirect: bool = False
+
     def __repr__(self):
         return 'Broken redirect (redirect.yaml:{}: {} --> {})'.format(
-            self.redirect_lineno, self.resolved_location.lower(), self.redirect_destination
+            self.redirect_lineno,
+            self.resolved_location.lower(),
+            link_parser.Link.colourise_location_static(*self.redirect_destination.split("#"), fragment_only=self._colourise_fragment_only_in_redirect)
         )
 
 
@@ -124,3 +128,30 @@ def __repr__(self):
     @property
     def pos(self):
         return self.link.fragment_start + 1
+
+
+class BrokenRedirectIdentifierError(
+    LinkError,
+    # TODO: would be cool to just inherit from these two
+    # BrokenRedirectError, MissingIdentifierError,
+    collections.namedtuple('BrokenRedirectIdentifier', 'link resolved_location redirect_lineno redirect_destination path identifier no_translation_available translation_outdated')
+):
+    """
+    An error indicating that a redirect points to a non-existent heading or identifier tag
+    that would produce such #identifier
+    """
+
+    link: link_parser.Link
+    resolved_location: str
+    redirect_lineno: int
+    redirect_destination: str
+    path: str
+    identifier: str
+    # for news posts, these two should be False
+    no_translation_available: bool # also implies a link to and from a translation
+    translation_outdated: bool
+
+    _colourise_fragment_only_in_redirect: bool = True
+
+    def __repr__(self):
+        return BrokenRedirectError.__repr__(self) + "\n" + MissingIdentifierError.__repr__(self)

wikitools/link_checker.py

@@ -61,7 +61,7 @@ def get_repo_path(
     Converts a wild link of into a osu-wiki repository path with an optional fragment, if possible
 
     Acceptable links can be in the following formats:
-    
+
     - Relative wiki link: path/to/article#optional-fragment
     - Absolute wiki link: /wiki/path/to/article#optional-fragment
     - News post link: https://osu.ppy.sh/home/news/2023-01-01-news#optional-fragment
@@ -109,7 +109,7 @@ def resolve_redirect(
     reference: typing.Optional[reference_parser.Reference],
     redirects: redirect_parser.Redirects,
     exists: typing.Callable[[pathlib.Path], bool]
-) -> typing.Union[RepositoryPath, errors.LinkError]:
+) -> typing.Union[typing.Tuple[RepositoryPath, str, int, str], errors.LinkError]:
     """
     Resolves a wiki article path according to redirects.
 
@@ -136,7 +136,7 @@ def resolve_redirect(
     if not exists(target_path.path):
         return errors.BrokenRedirectError(link, redirect_source, redirect_line_no, redirect_destination)
 
-    return target_path
+    return target_path, redirect_source, redirect_line_no, redirect_destination
 
 
 def check_link(
@@ -177,16 +177,18 @@ def check_link(
     if repo_path.path_type == PathType.GITHUB:
         exists = exists_case_sensitive
 
+    redirected = False
     if not exists(repo_path.path):
         # if the article doesn't exist, check if it has a redirect
         if repo_path.path_type == PathType.NEWS or repo_path.path_type == PathType.GITHUB:
             # except news and github links don't support redirects
             return errors.LinkNotFoundError(link, reference, repo_path.path.as_posix())
 
-        redirected_path = resolve_redirect(repo_path, link, reference, redirects, exists)
-        if isinstance(redirected_path, errors.LinkError):
-            return redirected_path
-        repo_path = redirected_path
+        redirect_result = resolve_redirect(repo_path, link, reference, redirects, exists)
+        if isinstance(redirect_result, errors.LinkError):
+            return redirect_result
+        repo_path, redirect_source, redirect_line_no, redirect_destination = redirect_result
+        redirected = True
 
     # link to an article in general, article exists -> good
     if not repo_path.fragment:
@@ -212,8 +214,7 @@ def check_link(
                 if repo_path.fragment not in target_article.identifiers:
                     # collect some additional metadata before reporting
                     translation_outdated = False
-                    if repo_path.path.name != "en.md":
-                        translation_outdated = target_article.front_matter.get('outdated_translation', False)
+                    if repo_path.path.name != "en.md": translation_outdated = target_article.front_matter.get('outdated_translation', False)
 
                     return errors.MissingIdentifierError(link, raw_path, repo_path.fragment, False, translation_outdated)
         case PathType.NEWS:
@@ -252,6 +253,20 @@ def check_link(
                         all_articles[raw_path_translation] = article_parser.parse(translation)
                     translation_outdated = all_articles[raw_path_translation].front_matter.get('outdated_translation', False)
 
+                # even an empty fragment in a redirect will take priority over the original link,
+                # so it's enough to just check for "#"
+                if redirected and "#" in redirect_destination:
+                    return errors.BrokenRedirectIdentifierError(
+                        link=link,
+                        resolved_location=redirect_source,
+                        redirect_lineno=redirect_line_no,
+                        redirect_destination=redirect_destination,
+                        path=raw_path,
+                        identifier=repo_path.fragment,
+                        no_translation_available=no_translation_available,
+                        translation_outdated=translation_outdated
+                    )
+
                 return errors.MissingIdentifierError(link, raw_path, repo_path.fragment, no_translation_available, translation_outdated)
 
     return None

wikitools/link_parser.py

@@ -99,12 +99,17 @@ def colourise_link(self, fragment_only=False):
         )
 
     def colourise_location(self, fragment_only=False):
+        return self.colourise_location_static(self.raw_location.split("#")[0], self.parsed_location.fragment, fragment_only=fragment_only)
+
+    # provided for convenience, used in `BrokenRedirectError`
+    @staticmethod
+    def colourise_location_static(location: str, fragment: typing.Optional[str] = None, fragment_only: bool=False):
         if fragment_only:
-            return "".join((
-                console.green(self.raw_location.split("#")[0]),
-                console.red('#' + self.parsed_location.fragment)
-            ))
-        return console.red(self.raw_location)
+            colourised_location = console.green(location)
+            if fragment:
+                colourised_location += console.red('#' + fragment)
+            return colourised_location
+        return console.red(location + ('#' + fragment if fragment else ""))
 
     def resolve(
         self, references: reference_parser.References

wikitools_cli/commands/check_links.py

@@ -143,7 +143,7 @@ def main(*args):
                 print(e.pretty_location(a.path, lineno))
             for e in errors_on_line:
                 print(e.pretty())
-                if isinstance(e, error_types.MissingIdentifierError):
+                if isinstance(e, error_types.MissingIdentifierError) or isinstance(e, error_types.BrokenRedirectIdentifierError):
                     suggestions = identifier_suggestions(e, articles)
                     if suggestions:
                         print('{}\n\t{}'.format(console.blue('Suggestions:'), suggestions))