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

Added linkcheck to docs build #24

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Added linkcheck to docs build #24

merged 2 commits into from
Nov 6, 2023

Conversation

niksirbi
Copy link
Member

@niksirbi niksirbi commented Nov 6, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
We often encounter broken links in our documentation websites, either because of typos, or because the links changed.

What does this PR do?
It adds an extra step to the build-docs action for checking the integrity of external links.
This step will be enabled by default, by can be deactivated via check-links: false (in case it produces annoying false positives for some projects)

References

neuroinformatics-unit/movement#80
neuroinformatics-unit/NeuroBlueprint#50

How has this PR been tested?

I tested the sphinx-build process locally. The real test will come after merging to main. If this fails, I should be able to tell via movement (which uses neuroinformatics-unit/actions/build_sphinx_docs@main).
After ensuring that movement behaves as intended, we can release this action, and after that, it will affect all repos that use neuroinformatics-unit/actions/build_sphinx_docs@v2.

Is this a breaking change?

Not per se, but it may lead to some docs build errors if some projects contain broken links. That's why I'm tagging @neuroinformatics-unit/neuroinformatics-all so that everyone is aware of this possibility and is not surprised.

You can debug the linkcheck step by running it locally:

sphinx-build docs/source docs/build -b linkcheck

If the linkcheck step produces "false positives" for your project (i.e. marking valid links as broken), you have two options:

  • Adding the following to conf.py (replace with the problematic URLs).
    # The linkcheck builder will skip verifying that anchors exist when checking 
    # these URLs (e.g. because they are generated by JavaScript).
    linkcheck_anchors_ignore_for_url = [
        "https://gin.g-node.org/G-Node/Info/wiki/",
        "https://neuroinformatics.zulipchat.com/",
    ]
  • Disabling the linkcheck step by setting the check-links input to false, e.g.:
    build_sphinx_docs:
      name: Build Sphinx Docs
      runs-on: ubuntu-latest
      steps:
      - uses: neuroinformatics-unit/actions/build_sphinx_docs@main
        with:
          check-links: false

Does this PR require an update to the documentation?

Documented in the relevant READMEs.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@niksirbi niksirbi marked this pull request as ready for review November 6, 2023 11:52
@@ -102,9 +102,12 @@ build_sphinx_docs:
- uses: neuroinformatics-unit/actions/build_sphinx_docs@main
with:
python-version: 3.10
check-links: false
Copy link
Contributor

Choose a reason for hiding this comment

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

default should be true?

Copy link
Member Author

Choose a reason for hiding this comment

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

the default is indeed true, but in the README usage example I show how you would deactivate it. Because it defaults to true, there is no point in including check-links: true when using the action (it's redundant). But if you think it's confusing I can either remove that line from the usage example, or change it to true (redundancy doesn't hurt in this case)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that makes sense. Let's keep it like this then.

Copy link
Member

@adamltyson adamltyson 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 @niksirbi. I haven't merged yet because you mentioned letting @neuroinformatics-unit/neuroinformatics-all take a look first.

I raised brainglobe/brainglobe.github.io#97 to track this within BrainGlobe. cc @alessandrofelder

@JoeZiminski
Copy link
Member

JoeZiminski commented Nov 6, 2023

This looks great! I don't have anything to add in terms of the implementation, happy to test this on other repos if required.

@niksirbi
Copy link
Member Author

niksirbi commented Nov 6, 2023

I think everyone who maintains Sphinx docs has weighed in, so I'll merge

@niksirbi niksirbi merged commit c1a6dc3 into main Nov 6, 2023
@niksirbi niksirbi deleted the update-docs-build branch November 6, 2023 15:57
@niksirbi
Copy link
Member Author

niksirbi commented Nov 6, 2023

Update

This appears to be working:

Shall I make a new release of the actions?

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

Successfully merging this pull request may close these issues.

4 participants