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

Add internal link checking to sphinx-build #1827

Merged
merged 20 commits into from
Feb 8, 2024
Merged

Conversation

stephprince
Copy link
Contributor

@stephprince stephprince commented Jan 17, 2024

Motivation

Fix #1731.

Running sphinx-build in nitpicky mode can be used to check internal links. This PR will update the GitHub workflow / Makefile to use nitpicky mode and fix broken links/references.

A couple of notes

  • Fixing all internal links will also require updates to hdmf (See related PR Fix internal links in docs hdmf-dev/hdmf#1031)

  • Running sphinx-build in nitpicky mode will generate warnings by default. If we want to be stricter, we can add the -W flag to generate errors for broken internal links

  • colons in docstrings get picked up by sphinx as references due to the sphinx Napoleon extension (See issue here). For now, I replaced colons with dashes, but there are a couple options for getting colons to work if it's preferred to keep them. See the image below for examples of what they might look like:
    Screenshot 2024-01-17 at 10 17 38 AM

  • Using type hints with h5py data types causes internal reference warnings. I couldn't figure out the best way to approach fixing that currently. It only happened in the get_nwbfile_version function so I switched it to use docval instead

How to test the behavior?

cd docs
make linkcheck

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (047c37d) 92.18% compared to head (dc74e36) 92.19%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1827   +/-   ##
=======================================
  Coverage   92.18%   92.19%           
=======================================
  Files          27       27           
  Lines        2637     2639    +2     
  Branches      689      690    +1     
=======================================
+ Hits         2431     2433    +2     
  Misses        136      136           
  Partials       70       70           
Flag Coverage Δ
integration 71.35% <100.00%> (+0.02%) ⬆️
unit 84.31% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stephprince stephprince marked this pull request as ready for review January 17, 2024 20:10
@stephprince stephprince requested a review from rly January 17, 2024 20:11
src/pynwb/__init__.py Outdated Show resolved Hide resolved
@oruebel
Copy link
Contributor

oruebel commented Jan 18, 2024

  • For now, I replaced colons with dashes

I think that's fine.

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Thanks for all these fixes. Aside from the issue with the mpi Intracom in docval this looks good to me.

docs/gallery/domain/images.py Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Jan 18, 2024

That's a lot of fixes! Thank you.

oruebel
oruebel previously approved these changes Jan 18, 2024
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

This looks all good to me. I would wait for @rly to approve as well, but I believe I need to approve too since I requested changes earlier.

@stephprince
Copy link
Contributor Author

did we want to raise nitpicky warnings as errors in the workflow here too?

@rly
Copy link
Contributor

rly commented Jan 19, 2024

did we want to raise nitpicky warnings as errors in the workflow here too?

Yes, that would be great.

It's too bad that the type hint for h5py.File doesn't play nicely with sphinx

@rly
Copy link
Contributor

rly commented Jan 19, 2024

Looks good to me. To be clear, the remaining warning will be fixed in the next release of HDMF, right?

/home/runner/work/pynwb/pynwb/src/pynwb/__init__.py:docstring of pynwb.docval.<locals>.dec.<locals>.func_call:9:py:class reference target not found: File

@rly
Copy link
Contributor

rly commented Jan 19, 2024

If so, let's hold off on merging until that's released.

rly
rly previously approved these changes Jan 19, 2024
@stephprince
Copy link
Contributor Author

Yes the remaining warnings should be fixed in the next release of HDMF. Right now it's stopping at the first warning (there are a lot more). If we want to show the rest of the errors in the workflow run we could add the --keep-going flag.

@rly
Copy link
Contributor

rly commented Feb 7, 2024

@stephprince It looks like there is a new failure in the build - could you please look into it today if you can? If not, let me know and I'll try. I would like this in the next pynwb release today/tomorrow.

@stephprince stephprince enabled auto-merge (squash) February 8, 2024 21:57
@stephprince stephprince merged commit f77f33c into dev Feb 8, 2024
23 checks passed
@stephprince stephprince deleted the internal-link-checking branch February 8, 2024 21:57
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.

[Feature]: Run sphinx build in nitpicky mode to check internal links
3 participants