From 09e5d285f461f5ac521524c88183f0b2ea52d982 Mon Sep 17 00:00:00 2001 From: Walavouchey <36758269+Walavouchey@users.noreply.github.com> Date: Thu, 15 Aug 2024 21:47:21 +0200 Subject: [PATCH] fix redirect error reporting --- tests/test_articles/wiki/redirect.yaml | 5 ++- .../wiki/redirected_sections/en.md | 6 +++ tests/test_link_checker.py | 39 ++++++++++++++++++- tests/visual/test_check_links.py | 7 +++- wikitools/errors.py | 35 ++++++++++++++++- wikitools/link_checker.py | 33 +++++++++++----- wikitools/link_parser.py | 15 ++++--- wikitools_cli/commands/check_links.py | 2 +- 8 files changed, 122 insertions(+), 20 deletions(-) create mode 100644 tests/test_articles/wiki/redirected_sections/en.md diff --git a/tests/test_articles/wiki/redirect.yaml b/tests/test_articles/wiki/redirect.yaml index bd4de37..c1fcb82 100644 --- a/tests/test_articles/wiki/redirect.yaml +++ b/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 diff --git a/tests/test_articles/wiki/redirected_sections/en.md b/tests/test_articles/wiki/redirected_sections/en.md new file mode 100644 index 0000000..5b0f19d --- /dev/null +++ b/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) diff --git a/tests/test_link_checker.py b/tests/test_link_checker.py index 6aa40f6..03e4f76 100644 --- a/tests/test_link_checker.py +++ b/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( diff --git a/tests/visual/test_check_links.py b/tests/visual/test_check_links.py index 5fc785a..09db06e 100644 --- a/tests/visual/test_check_links.py +++ b/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") + ), ] ) diff --git a/wikitools/errors.py b/wikitools/errors.py index 1e768af..798adfc 100644 --- a/wikitools/errors.py +++ b/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) diff --git a/wikitools/link_checker.py b/wikitools/link_checker.py index 5ee6d4d..c2e1db2 100644 --- a/wikitools/link_checker.py +++ b/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 diff --git a/wikitools/link_parser.py b/wikitools/link_parser.py index 7c48c3c..38ffdf9 100644 --- a/wikitools/link_parser.py +++ b/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 diff --git a/wikitools_cli/commands/check_links.py b/wikitools_cli/commands/check_links.py index 5efc310..57c9850 100644 --- a/wikitools_cli/commands/check_links.py +++ b/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))