Skip to content

Conversation

fingolfin
Copy link
Member

The sources entry in docs/Project.toml helps ensure the correct version of the package is used.

There is also now a way to override the doctest test setting, so that one can turn them off for a quick test, or enable the fix mode for updating examples.

This may need some discussion before it gets merged and rolled out to perhaps some more of our repos; I'll add some inline comments to relevant code sections

The `sources` entry in `docs/Project.toml` helps ensure
the correct version of the package is used.

There is also now a way to override the `doctest` test setting,
so that one can turn them off for a quick test, or enable the
`fix` mode for updating examples.
Documenter = "1"

[sources]
AbstractAlgebra = {path = ".."}
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 believe this is an unambiguous win and can be merged.

# DOCTEST=off julia --proj=docs docs/make.jl
#
using Pkg
Pkg.update()
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is debatable I guess, and perhaps indeed clearly bad (as it may e.g. hinder CI). The main reason I put it in is because I feel forgetting to run update in your docs environment is a common cause of issues.

Perhaps it should be changed to instantiate or entirely dropped. If we drop it, I think we should at least put in language into the comment above that recommends running the update manually in that environment on occasion.

Or, we could keep it, but add a way to control it via a command argument and/or environment var, just like the doctest. (And what the default value would be could be debated).

Copy link
Member

Choose a reason for hiding this comment

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

In the past, I used to dev the main package into the docs environment. This then did not need any updating of the docs environment at all. (will look more in-depth at this PR later)

Comment on lines +19 to +22
if "--fix" in ARGS
doctest_arg = :fix
else
tmp = get(ENV, "DOCTEST", "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.

I think it is overkill to use both an env variable and and a command line argument to turn on "fix" mode. Thing is, I started with the idea of adding a command line argument, and just --fix, but then realized I also want to be able to turn off the doctests entirely. And then I got a bit lazy, as I'd have to decide what the args should be named (--doctest=on/off/fix?) and how to parse it ;-).

I think overall I'd find a command line argument nicer, and would be willing to implement it properly, but before I spend effort on it, I'd like to find out what others think.

Copy link

codecov bot commented Sep 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.93%. Comparing base (4b127f8) to head (f51792d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2173   +/-   ##
=======================================
  Coverage   87.93%   87.93%           
=======================================
  Files         127      127           
  Lines       31776    31776           
=======================================
  Hits        27941    27941           
  Misses       3835     3835           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thofma
Copy link
Member

thofma commented Sep 22, 2025

What about the Hecke.build_doc()/Oscar.build_doc() way? Just wondering what you mean when you say "rolled out to perhaps some more of our repos". Do we want two slightly different ways for the same thing?

@fingolfin
Copy link
Member Author

I don't like the Hecke.build_doc()/Oscar.build_doc() way -- it causes us all kinds of pain, and I think we can get the same or better with less hacks, based on what this PR does plus more.

@fingolfin
Copy link
Member Author

(In particular I have an extended version of this using Revise and for repeated rebuilds of the docs. I'll try to get it cleaned up and into another PR, but I think it might actually be better to do one for Hecke or Oscar first. We'll see. Might also be a good idea if @thofma @lgoettgens and me sat on a Zoom (?) call for a bit to talk about it. But we are all busy and this is hardly a priority right now...

@fieker
Copy link
Contributor

fieker commented Sep 26, 2025

What is is "new" idea how to build the documentation? As in how and what command?

@thofma
Copy link
Member

thofma commented Sep 26, 2025

Without Revise support I have a hard time imagening how doing julia docs/make.jl (which is what this PR here adivses?) is better than *.build_doc(). Can you just add the Revise support here in this PR?

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