-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for broken links in the CI test job #369
base: main
Are you sure you want to change the base?
Conversation
7a69033
to
4ca0c0e
Compare
The most common error I see is linking to a page that exists internally in swift.org, but the specific heading does not. One specifically annoying scenario is when a blog post has heading links to parts of that post in excerpt. The excerpt is rendered in the blog index page, so we will render those links, but the headings only exist in the specific article page. For example: https://www.swift.org/blog/ has a link to To fix those, we will need to fix them in the source markdown, just prepend |
408 link failures
|
d7f9503
to
5884f9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an example of how we can resolve one "family" of issues — if the general direction of this seems useful, I will work to isolate the actual links leading to 404 and fix those first.
@@ -6,7 +6,7 @@ title: Swift 5.8 Released! | |||
author: [alexandersandberg] | |||
--- | |||
|
|||
Swift 5.8 is now officially released! 🎉 This release includes major additions to the [language and standard library](#language-and-standard-library), including `hasFeature` to support piecemeal adoption of upcoming features, an improved [developer experience](#developer-experience), improvements to tools in the Swift ecosystem including [Swift-DocC](#swift-docc), [Swift Package Manager](#swift-package-manager), and [SwiftSyntax](#swiftsyntax), refined [Windows support](#windows-platform), and more. | |||
Swift 5.8 is now officially released! 🎉 This release includes major additions to the [language and standard library]({% link _posts/2023-03-30-swift-5.8-released.md%}#language-and-standard-library), including `hasFeature` to support piecemeal adoption of upcoming features, an improved [developer experience](#developer-experience), improvements to tools in the Swift ecosystem including [Swift-DocC](#swift-docc), [Swift Package Manager](#swift-package-manager), and [SwiftSyntax](#swiftsyntax), refined [Windows support](#windows-platform), and more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of how we can use a liquid helper to build a link, achieving two things:
- Since this link includes relative path to this article, when excerpt gets rendered on
/blog/
page, the link would still be valid. {% link %}
performs a validation when it's rendering. If the resulting link is invalid, it won't render, and the generation will fail.
The amount of work to change all links lime that would be colossal, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can add a custom tag, something like anchor_url(to_post=post.slug, anchor)
and then just search and replace links in posts where <a href="#{whatever}"
.
That way, it would be easier to convert them all in one pass.
cc: @shahmishal |
ed90a60
to
10145c3
Compare
With the footer links fixed in 61807b7, we're down to 95 errors: Here they are
This is manageable, I can power through them, so we start with a clean slate when we merge this (if we choose to merge this at all, no hard feelings if this is not the direction we would like to adopt). |
thanks @natikgadzhi that would be very useful |
61807b7
to
bd0325e
Compare
@swift-ci test |
bd0325e
to
520f823
Compare
@shahmishal okay, good news / bad news:
I also tried to keep each individual commit meaningful, so I would suggest not squashing them in case we'll need to blame / review this later. I think we can merge this and wire up the check into the CI and GitHub checks, and I can hunt down the rest of the broken links in batches. How do you feel about that approach? |
- This commit is part of the work to clean up all our broken links in swiftlang#369. This can be shipped on it's own, so I'm separating it out to keep the main PR as small as possible. - This commit replaces links to `https://lists.swift.org/mailman/listinfo` with `https://lists.swift.org/pipermail/` - In cases where we guide users to submit feedback, I've also added a `> Note` that explains that mailing lists are now closed and archived, and they should use forums instead.
67ed0a0
to
cb037aa
Compare
@shahmishal, this PR is in decent shape I think — we can merge this and I will work through the list in separate follow-up PRs. That way, you can wire up the CI check Until you have a moment to do this, I'll contribute more fixes in here. I'm separating fixes if they span multiple files, and keeping simple fixes in this PR, but in separate commits, so it's easy to review commit by commit. |
This commit fixes 4 more broken links: the navigation menu item, the reference-less link, and two other blog posts. The `_navigation.yml` and several pages linked to `/contributing/#evolution-process`, and we had an `<a>` tag with `name="evolution-process"`, which is invalid in `htmlproofer`s eyes because it's a link that does not have a reference. Jekyll makes `id`s for every heading automatically, and we have `/contributing/#swift-evolution` that we should link to instead.
The 404.html page renders with HTTP404 code, but that's valid. Hence, we should ignore that page in `check_links.sh`
@swift-ci test links |
Let me know if I got something wrong in the setup. |
We have just one link without the `href` attribute, which is a button in the Swift Evolutions browser. We know it's valid, and the case where `<a>` tags won't have `href`s is rare, so we can safely ignore them in the check.
In 0317303, I've added `htmlproofer` to `Gemfile`, but did not commit the changes to `Gemfile.lock`. This commit adds it back.
51de20d
to
42a8209
Compare
@shahmishal, thank you for setting it up! That was my mistake, just pushed a fix. |
@swift-ci test links |
1 similar comment
@swift-ci test links |
It's alive! The check works correctly now, 101 failures sounds about right. I'm slowly making my way through that list. |
- This commit is part of the work to clean up all our broken links in #369. This can be shipped on it's own, so I'm separating it out to keep the main PR as small as possible. - This commit replaces links to `https://lists.swift.org/mailman/listinfo` with `https://lists.swift.org/pipermail/` - In cases where we guide users to submit feedback, I've also added a `> Note` that explains that mailing lists are now closed and archived, and they should use forums instead.
@natikgadzhi First, thank you for all the work you are doing on this! Your previous comments regarding twitter.com links inspired me to have us prefer GitHub profiles to Twitter/X profiles in PR #408 and #409. I believe once #409 is merged, it should be possible to tighten up the exclusion for twitter.com links and only need to exclude:
Hopefully that will help us to avoid adding new links to profiles that require a sign-in. |
@swift-ci test links |
Upd: this is still in progress, and I fully intend to get this to fully clean state with zero broken links per linter. But, this and last week were pretty full for me, hence the slowdown. I'll be back to work (on this) on Friday! |
This comment was marked as resolved.
This comment was marked as resolved.
@natikgadzhi - hey out there, are you still interested in working on this? Thanks! |
@parispittman I have not looked at this in a while. I'm happy to help dust this off and rebase over master and make a loom overview, and perhaps we can figure out a good path forward? It looked like everyone wants this, but it's becomming a big PR that is challenging to manage. We should probably chunk it up into a series of smaller ones. Does that sound about right? |
@natikgadzhi, please let me know if I can help with anything to get this merged. It would be a great addition to the testing suite. Also, @sahilshahdesign is interested in contributing to the website, so if you have a few tasks he can help with on this PR, please let us know. Thanks! |
Adds
htmlproofer
check to make sure there are no broken links in the built website.Motivation:
Modifications:
gem html-proofer
alongside Jekyll. Since the website is already using Ruby, it's probably the easiest way to add broken links check.docker-compose.yml
, so hopefully this would just work with the existing CI setup.Review and concerns
There are 408 reported errors
//domain.tld
links that use the page protocol without enforcinghttps://
specifically. We should probably just ignore them, or check if the fix is in some sort of alink_tag
in a template so we can fix it quickly.htmlproof configuration
Below are options we can consider tinkering with — defualts are more or less okay, I think. here's what I'm using:
htmlproof
enforces https by default and complaints about a lot of links,--no-ensure-https
did not remove those from output.--only-4xx
to only surface HTTP 4xx errors.--ignore-urls
set to ignore twitter and github links. Github rate limits us, and Twitter just plain does not work anymore without authentication headers.--ignore-missing-alt
checksimg
tags and verifies they havealt
. It's on by default.TODOs
alt
attributes