Skip to content
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 docs links after building #2024

Merged
merged 4 commits into from
May 21, 2024

Conversation

blairconrad
Copy link
Member

No description provided.

Target(
"generate-docs",
() => Run("python", "-m mkdocs build --clean --site-dir artifacts/docs --config-file mkdocs.yml --strict"));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I waffled over how to structure the targets.
Coulda been one big target: build and check.
Coulda been 3 targets: docs that depends on check-docs-links that depends on generate-docs
Coulda been 3 targets: docs that depends on both generate-docs and check-docs-links (with checks-docs-links depending on nothing)

I went this way but would go another way!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer this one:

3 targets: docs that depends on check-docs-links that depends on generate-docs

It's a bit strange that the docs target actually does the check

Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @blairconrad, nice work as usual! I have some minor comments

how_to_build.md Show resolved Hide resolved
Comment on lines 59 to 64
// If mkdocs's site_url configuration is not set, sitemap.xml is full of "None" links.
// Even with a valid site_url, the link check would fail if we added a new page.
// So trust that sitemap.xml will be generated correctly.
// The 404.html page is likewise generated, and isn't even the one we use on the live site (mike generates a new one).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this comment. If I understand correctly, because site_url is not set, the sitemap.xml is not generated correctly. But then you say that we "trust that sitemap.xml is generated correctly"?

Copy link
Member Author

@blairconrad blairconrad May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point.

// If mkdocs's site_url configuration is not set, sitemap.xml is full of "None" links.
// Even with a valid site_url, the link check would fail if we added a new page.
// So trust that any non-None links that mkdocs puts in sitemap.xml will be valid.
// The 404.html page is likewise generated, and isn't even the one we use on the live site (mike generates a new one).

?

Target(
"docs",
DependsOn("generate-docs"),
() => Run("linkchecker", "--ignore-url=sitemap.xml --ignore-url=404.html --check-extern -F html/./artifacts/docs-link-check.html ./artifacts/docs/index.html"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the -F option do? Path of the report? Why /./?

Copy link
Member Author

@blairconrad blairconrad May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format and path of the report. The /./ is probably there because, I dunno. I got the filename via path completion in the shell and left it. Removing.

 -F TYPE[/ENCODING[/FILENAME]], --file-output TYPE[/ENCODING[/FILENAME]]

    wrap each of the following lines to match the length of the first one
    Output to a file linkchecker-out.TYPE, $XDG_DATA_HOME/linkchecker/failures for 'failures' output,
    or FILENAME if specified. The ENCODING specifies the output encoding, the default is that of your
    locale. Valid encodings are listed at https://docs.python.org/library/codecs.html#standard-encodings.
    The FILENAME and ENCODING parts of the 'none' output type will be ignored, else if the file already
    exists, it will be overwritten. You can specify this option more than once.
    Valid file output types are 'csv', 'xml', 'dot', 'failures', 'gml', 'gxml', 'html', 'none',
    'sitemap', 'sql', 'text'. You can specify this option multiple times to output to more than one file.
    Default is no file output. Note that you can suppress all console output with the option '-o none'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is ENCODING? You just did TYPE/FILENAME, is that valid? How does it know whether it's the encoding or the filename?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be valid. I dunno. It sussed it out. I'll add an encoding.

.github/workflows/ci.yml Show resolved Hide resolved
Target(
"generate-docs",
() => Run("python", "-m mkdocs build --clean --site-dir artifacts/docs --config-file mkdocs.yml --strict"));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer this one:

3 targets: docs that depends on check-docs-links that depends on generate-docs

It's a bit strange that the docs target actually does the check

@blairconrad
Copy link
Member Author

blairconrad commented May 21, 2024

Thanks for comments, @thomaslevesque. Hopefully resolved. If not, do tell me! Rebased. All commits but the last applied cleanly, and it contains changes to address your comments.

@blairconrad
Copy link
Member Author

Late-breaking update. Upgraded the documentation dependencies to go along with our new requirement. 'sides, there's a new requests since you merged dependabot's PR yesternight.

Can drop if you like.

Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @blairconrad!

@blairconrad
Copy link
Member Author

Thanks, @thomaslevesque.

@thomaslevesque thomaslevesque merged commit 6e18673 into FakeItEasy:master May 21, 2024
4 checks passed
@blairconrad blairconrad added this to the vNext milestone May 21, 2024
@blairconrad blairconrad deleted the check-docs-links branch May 23, 2024 16:31
@afakebot
Copy link

This change has been released as part of FakeItEasy 8.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants