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 Furo theme to the documentation build #70

Merged
merged 10 commits into from
Aug 17, 2023
Merged

Conversation

mscheltienne
Copy link
Collaborator

@mscheltienne mscheltienne commented Aug 16, 2023

Closes #68

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

A couple of information: it does build and look OK, but with warnings. The same warnings are present on the main branch with sphinx_rtd_theme anyway, so I guess you did not configure the CI build to fail on warnings. Then a couple of words on what I changed/added beside the theme:

  • Added the light/dark logos (very nice ones!) in SVG format to the docs/_static/ folder. Personally, I would store all logos there anyway and point to that location where it's needed, e.g. in the README.md. Let me know if you want a different logo version instead of the light/dark I used.

  • Added a couple of extensions: mathjax for mathematical expressions, bibtex for bibliography, copybutton, design and issues. It's a personal default set, let me know if you want to remove some.

    • bibtex requires a .bib file listing the bibliography anyway, so for now it's not active.
    • mathjax renders mathematical expressions, e.g. :math:\frac{\partial{B_x}}{\partial{x}} renders as: Screenshot from 2023-08-16 15-47-54
    • copybutton adds a button to copy the content of a code-block, super useful!
    • design adds a couple of directives for tiles, grids, cards, dropdown, ... https://sphinx-design.readthedocs.io/en/latest/ It's easy to use and looks very nice.
    • issues adds an issue and pr directive to link to GitHub, very useful in a changelog. e.g.
    - change XXX by YYY in :pr:`101`
    
  • Changed the landing page index by:

    • adding :hide-toc: to remove the right-side menu which only contained Cite (only subtitle present)
    • edited the depth of the toctree to sensible default, let me know if you want something different
    • remove the "package" toctree which was basically empty and pointing back to the page you are already on

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #70 (f4f8351) into master (8e4fbae) will not change coverage.
Report is 3 commits behind head on master.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #70   +/-   ##
=======================================
  Coverage   90.93%   90.93%           
=======================================
  Files          14       14           
  Lines        1235     1235           
=======================================
  Hits         1123     1123           
  Misses        112      112           
Files Changed Coverage Δ
nigsp/io.py 87.42% <ø> (ø)
nigsp/operations/metrics.py 100.00% <ø> (ø)
nigsp/operations/surrogates.py 89.91% <ø> (ø)
nigsp/operations/timeseries.py 95.37% <ø> (ø)
nigsp/utils.py 95.12% <ø> (ø)

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

Very neat, I like it!

Question for @NawalK:
Which logo should we use in which environment? Currently this is the status:

Schermata da 2023-08-17 08-46-53
Schermata da 2023-08-17 08-46-41

Should we switch them/adopt coral instead?

Also @mscheltienne, since you were talking changelogs and you're modifying the documentation, can you add the current changelog as an item of the documentation, please?
It'd be great in "Usage" after "Licence".

@mscheltienne
Copy link
Collaborator Author

Usually, the changelog would be defined per version, e.g. for scipy: https://docs.scipy.org/doc/scipy/release.html
But it doesn't work well with the autorelease system you have here. Maybe an external link to the GitHub release page https://github.com/MIPLabCH/nigsp/releases would be better?

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

Sounds fair enough - I am fairly attached to the autorelease system tbh, so I prefer having that (also, we're at a much less complex stage than scipy for our changelog to be fairly small).

@mscheltienne
Copy link
Collaborator Author

I am fairly attached to the autorelease system tbh

Seems to be working great, and it's easy to follow!

@mscheltienne
Copy link
Collaborator Author

A couple more points/ideas:

  • I'd suggest to fix the warnings emitted during the doc-build, if they accumulate it can be very annoying to fix at a later stage.
  • You can also enable the nitpick mode to warn on all missing/invalid references.
nitpicky = True
nitpick_ignore = []
  • I prefer numpydoc to sphinx.ext.napoleon. It comes with better docstring validation and with a system of xref aliases, e.g. {"Axes": "matplotlib.axes.Axes"} is defined, the docstring:
def foo(ax: Axes) -> None:
    """doc

    Parameters
    ----------
    ax : Axes
        Matplotlib axes on which foo operates.
    """
    pass

will render with a link to the matplotlib documentation while it will not if sphinx.ext.napoleon is used.

  • You don't have tutorials/examples yet I think, but when you do, I'd suggest using sphinx-gallery. If you want, I can add a sane default configuration + css style sheets in this PR.

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

Beside the warnings (unless you want to take care of them), yes to all of the above!

Also re:logos, could you use the circle_coral ones on both light and dark?

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

@mscheltienne let me know when this PR is ready for review!

@mscheltienne
Copy link
Collaborator Author

Logo changed, looks better!

I added nitpick, numpydoc, sphinx-gallery. It build with about 170 warnings, I brought it down to 65 remaining warnings. I'll let you fix the remaining ones, I was a bit lazy to go through all the files/docstrings ;)
For sphinx-gallery, the configuration is set to run tutorials with a name starting with 2 digits, e.g. 00_tutorial_1, 10_tutorial_2, ... and to sort the tutorial by those digits. The idea is that you can now create your tutorials 00_, 10_, 20_, .. in a sensible order and you can add later a 05_ tutorial between 0 and 10 if needed.
Also, the small custom CSS file hides the note at the top of every tutorial:

Screenshot from 2023-08-17 13-59-39

It's not very useful, especially above the title.


If you are not familiar with some of those extensions, let me know if you have any questions on the configuration; and feel free to push directly to this PR if you need.

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

Thank you Mathieu! Can I merge this PR then (I won't fix warnings anyway 😅)?

nigsp/utils.py Outdated Show resolved Hide resolved
@mscheltienne
Copy link
Collaborator Author

Yes, I think it's good to merge, but check maybe quickly the documentation render: https://nigsp--70.org.readthedocs.build/en/70/
You are familiar with it, you might spot something ;)

@smoia
Copy link
Collaborator

smoia commented Aug 17, 2023

I checked it locally, seems good!

Copy link
Collaborator

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGTM!

@smoia smoia merged commit 4ffa7cb into MIPLabCH:master Aug 17, 2023
8 checks passed
@smoia smoia added the Documentation This issue or PR is about the documentation label Aug 17, 2023
@mscheltienne mscheltienne deleted the doc branch August 17, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation This issue or PR is about the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A nice website theme
2 participants