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.

Sign up for GitHub

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

Jump to bottom

Fix linkcheck for docstrings #2330

Merged
merged 4 commits into from
Nov 8, 2023
Merged

Fix linkcheck for docstrings #2330

merged 4 commits into from
Nov 8, 2023

Conversation

goerz
Copy link
Member

@goerz goerz commented Nov 3, 2023 •
edited
Loading

Dispatch Documenter.linkcheck on the type of node.element, allowing it to process a Documenter.DocsNode

Closes #2329

This was referenced Nov 3, 2023
Merged
Closed
@goerz goerz marked this pull request as ready for review November 3, 2023 19:43
@goerz
Copy link
Member Author

goerz commented Nov 3, 2023

This solves the problem that links inside docstrings were not being checked.

As soon as this is merged and released, I'll be able to address JuliaDocs/DocumenterCitations.jl#58

I haven't done any of the further refactoring that I proposed #2329 (comment). It's probably best to keep this PR minimal and get it out the door.

Maybe I can do the refactoring in a separate PR at some point in the future. The tests I've included in this PR are okay because I'm testing with non-existent domains, which are always going to fail (even if the CI doesn't have network access). If we wanted to do more elaborate testing of the linkcheck function, e.g., for links with different return codes, we'd definitely need to be able to mock out the calls to curl, or any actual network access.

@goerz goerz mentioned this pull request Nov 3, 2023
Closed
@goerz goerz force-pushed the mg/docstring-linkcheck branch 2 times, most recently from ac62228 to 6c474d4 Compare November 3, 2023 20:08
@goerz goerz mentioned this pull request Nov 3, 2023
Merged
mortenpi
mortenpi approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@mortenpi mortenpi 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 for catching this! This is a pretty bad regression.

As for the refactoring: yea, I think it would be better to do that in a separate PR, as not to tangle up the fix and refactor.

src/docchecks.jl Show resolved Hide resolved
src/docchecks.jl Outdated Show resolved Hide resolved
mortenpi
mortenpi reviewed Nov 6, 2023
View reviewed changes
src/docchecks.jl Outdated Show resolved Hide resolved
mortenpi
mortenpi reviewed Nov 6, 2023
View reviewed changes
test/runtests.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi self-requested a review November 6, 2023 05:34
@goerz
Move test for links in docstrings into test/online_linkcheck.jl
b74778c 
The test in `test/docstring_links/make.jl` now longer runs are part of
the standard test suite. Instead, CI runs it independently, in the
`linkcheck` step.
@goerz
Include test/online_linkcheck.jl in coverage
a453c44
@goerz
Copy link
Member Author

goerz commented Nov 7, 2023

Okay, should be good for merging or further review.

The coverage went down from moving the test into online_linkcheck.jl, so I added coverage collection to that CI job, in the last of the commits.

@mortenpi mortenpi merged commit 715a885 into JuliaDocs:master Nov 8, 2023
21 checks passed
@mortenpi
Copy link
Member

mortenpi commented Nov 8, 2023 •
edited
Loading

LGTM, thanks @goerz!

@goerz goerz deleted the mg/docstring-linkcheck branch November 8, 2023 01:01
@mortenpi mortenpi mentioned this pull request Nov 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortenpi mortenpi Awaiting requested review from mortenpi

Assignees
No one assigned
Labels
Type: Bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Linkcheck does not traverse into docstrings
2 participants
@goerz @mortenpi