Add AbstractAlgebra.doctestsetup() helper and use it, then regenerate doctests so they benefit from @show_name
#1895
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1895 +/- ##
==========================================
+ Coverage 88.14% 88.16% +0.02%
==========================================
Files 120 120
Lines 30288 30296 +8
==========================================
+ Hits 26697 26711 +14
+ Misses 3591 3585 -6 ☔ View full report in Codecov by Sentry. |
|
weird, I did |
fingolfin
force-pushed
the
mh/doctestsetup
branch
from
4957599
to
e210ed5
Compare
| @@ -15,7 +15,7 @@ as the two names really refer to the same object. | |||
| The alias also gets a special docstring that points out that it is an alias | |||
|
|
|||
| # Examples | |||
| ```jldoctest; setup = :(using AbstractAlgebra) | |||
| ```jldoctest; setup = :(using AbstractAlgebra; AbstractAlgebra.set_current_module(@__MODULE__)) | |||
There was a problem hiding this comment.
I would prefer if this inline setup thing would not duplicate the content of doctestsetup() into this many files. Can't you call doctestsetup() instead here? (with some interpolation magic maybe)
There was a problem hiding this comment.
The inline setup should not be necessary at all when setdocmeta! is called for :DocTestSetup. We don't do any setup like this in Oscar.jl, except for some artifacts downloads.
(The DocTestSetup = AbstractAlgebra.doctestsetup() are still required for the md files as this is independent from setdocmeta!)
There was a problem hiding this comment.
(I was actually wondering why we need to do this for every (?) docstring.)
There was a problem hiding this comment.
We don't use setdocmeta! because then Nemo and Oscar would start to break. This is explained at the top of docs/make.jl, this PR slightly improves that explanation. Here it is for your convenience:
# We do *not* call `DocMeta.setdocmeta!` with value `:(using AbstractAlgebra)`
# because we want to force that each jldoctest has its individual
# `setup = :(using AbstractAlgebra)`.
# The reason is that the AbstractAlgebra doctests are run also in Nemo,
# where `using AbstractAlgebra` must be avoided, as a few symbols change
# their meaning, e.g. `QQ`.
If you inspect the final commit in this PR you'll see that I previously tried to do setup = AbstractAlgebra.doctestsetup() but it just didn't work.
Of course I'd also prefer if we could do something like this, or even better, use setdocmeta!, but any solution must keep the Nemo and Oscar doctests working.
But unless someone has a bright idea quickly, I'd still like to merge this as-is, for the improved doctest contents, and then I'll happily review someone else's review for resolving this in a better way :-)
There was a problem hiding this comment.
(E.g. perhaps it suffices to only apply the explicit setup block to doctests which reference ZZ or QQ or so. But I'd rather not spend more hours figuring that out just now)
There was a problem hiding this comment.
I see, thanks for the explanation. I did see a setdocmeta! at the beginning of the diff but now realized that this is only in the CI.yml.
And the # We do *not* call `DocMeta.setdocmeta!` line was unfortunately hidden in the diff.
Fine with me for now.
There was a problem hiding this comment.
I wonder if this can be resolved later by changing Nemo to do
DocMeta.setdocmeta!(Nemo, :DocTestSetup, :(using Nemo); recursive = true)
DocMeta.setdocmeta!(Nemo.AbstractAlgebra, :DocTestSetup, Nemo.AbstractAlgebra.doctestsetup(); recursive = true)
(And then similar in Hecke, Oscar). I'll open an issue as a reminder to figure this out.
fingolfin
changed the title
@show_name
|
|
||
| julia> D, f = direct_sum(S1, S2, S3) | ||
| (DirectSumModule over integers, AbstractAlgebra.Generic.ModuleHomomorphism{BigInt}[Hom: submodule over integers with 2 generators and no relations -> DirectSumModule, Hom: submodule over integers with 2 generators and no relations -> DirectSumModule, Hom: submodule over integers with 2 generators and no relations -> DirectSumModule], AbstractAlgebra.Generic.ModuleHomomorphism{BigInt}[Hom: DirectSumModule -> submodule over integers with 2 generators and no relations, Hom: DirectSumModule -> submodule over integers with 2 generators and no relations, Hom: DirectSumModule -> submodule over integers with 2 generators and no relations]) | ||
| (DirectSumModule over integers, AbstractAlgebra.Generic.ModuleHomomorphism{BigInt}[Hom: S1 -> D, Hom: S2 -> D, Hom: S3 -> D], AbstractAlgebra.Generic.ModuleHomomorphism{BigInt}[Hom: D -> S1, Hom: D -> S2, Hom: D -> S3]) |
There was a problem hiding this comment.
This kind of change is what my true goal here is
... similar to what we do in Oscar. Arguably the test outputs in docstrings are better this way for the user, as they look more like what they would be seeing.
If successful we should probably do the same in Nemo (and Hecke?)