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

doctest and CitationBibliography #34

Closed
Sbozzolo opened this issue Sep 6, 2023 · 11 comments · Fixed by #43
Closed

doctest and CitationBibliography #34

Sbozzolo opened this issue Sep 6, 2023 · 11 comments · Fixed by #43
Labels
enhancement New feature or request

Comments

@Sbozzolo
Copy link

Sbozzolo commented Sep 6, 2023

Consider the minimum example

using Documenter
using Documenter: doctest
using DocumenterCitations

doctest(Documenter)

# makedocs(
#    CitationBibliography(joinpath(@__DIR__, "bibliography.bib")),
#  ...
#

The returns the following error (but continues processing)

 Error: You are `using CitationBibligraphy`, but did not pass the plugin to `makedocs`

As far I can tell, there's no way to pass CitationBibliography to doctest. I am not sure what should be done in this case, but demoting the error to a warning seems like a good first step.

@goerz
Copy link
Member

goerz commented Sep 6, 2023

I can certainly make it a warning, but it might be worth looking a little deeper. At least in principle, I feel like doctest should be able to take plugins into account. For citations, it doesn't really matter, but I could imagine other plugins producing something that would affect testing.

@mortenpi Do you have any general comments on how doctests and plugins should interact?

@goerz
Copy link
Member

goerz commented Sep 6, 2023

Looking at

https://github.com/JuliaDocs/Documenter.jl/blob/d9db9071d596cb9be81e742789952f2da2de8980/src/Documenter.jl#L942-L949

I feel like this should probably be considered a bug in doctest. It would be good if doctest could get a keyword argument plugins=[list_of_plugins...] and forward that to makedocs there.

@goerz
Copy link
Member

goerz commented Sep 6, 2023

Opened JuliaDocs/Documenter.jl#2245

@goerz
Copy link
Member

goerz commented Sep 6, 2023

Looks like we'll address this in Documenter in the next release.

I'm somewhat inclined to leave the @error (and close this issue). The problem is that just using CitationBibilography already inserts the citation logic into Documenter's pipeline, and that logic needs access to the plugin object. It's probably catchable in a way that's recoverable enough for doctest to run through, but at some level, any call to makedocs (which doctest calls) without the plugin object is pretty fundamentally broken.

How much of a blocker is this for you? Can you wait for Documenter 1.0 (and #3)? Would just changing @error to @warn be a sufficient solution? That is, can you dev DocumenterCitations and try it with ClimaAtmos?

@Sbozzolo
Copy link
Author

Sbozzolo commented Sep 6, 2023

Thank, @goerz, so much for this!

The reason I was suggesting moving to warning the execution continued after the error (which is not what I would expect from errors). In my particular case, the error is indeed harmless because the information is not used in doctest. For this reason, this is not a big issue for us and we can certainly wait for Documenter 1.0.

Thanks again!

@goerz
Copy link
Member

goerz commented Sep 10, 2023

Downstream fix: JuliaDocs/Documenter.jl#2249

@goerz
Copy link
Member

goerz commented Sep 11, 2023

Having implemented JuliaDocs/Documenter.jl#2249 gave me some more detailed insight into how doctest works internally, and I might revise CitationBibliography further to avoid the error message in the context of a doctest run, even is someone calls doctest without the new plugins keyword.

However, just to clarify: Does it actually make a practical difference whether the warning is generated with @warn vs with @error? I think as far as makedocs/doctest goes, the behavior is exactly the same, no? Is there any difference with how a @warn/@error interacts with the strict keyword argument in makedocs (warnonly in Documenter 0.28/1.0)?

To get to the core of my question:

@Sbozzolo:

The reason I was suggesting moving to warning [is that] the execution continued after the error

That's a personal preference, though, right? – And I'm not sure I agree. I've been using the following mental model:

  • error(msg): Fatal error, execution cannot continue
  • @warn msg: There's something that needs the user's attention (but it might be ok)
  • @error msg: There's something wrong. We can keep running, but the user is definitely not getting the result they're expecting.

So in some sense @error is just a @warn with more emphasis ("you can't just ignore this"), but not necessarily a fatal error. In DocumenterCitations, I've been considering, e.g., missing citation references (whether due to forgetting to specify a .bib file or due to citing a non-existing keys) as errors, while, e.g., malformed bibtex entries are warnings.

One reason I often use non-fatal @error messages: It can be useful to get multiple errors (e.g., multiple missing references), so that the user can fix all of the underlying issues before rerunning the program. With fatal errors, you often get a tedius "run, error, fix" loop.

It would be interesting to know if there's a general consensus in the community on whether @error messages are expected to stop execution.

@goerz
Copy link
Member

goerz commented Sep 11, 2023

See https://discourse.julialang.org/t/semantics-of-error/103760 for the philosophical discussion

@Sbozzolo
Copy link
Author

Having implemented JuliaDocs/Documenter.jl#2249 gave me some more detailed insight into how doctest works internally, and I might revise CitationBibliography further to avoid the error message in the context of a doctest run, even is someone calls doctest without the new plugins keyword.

However, just to clarify: Does it actually make a practical difference whether the warning is generated with @warn vs with @error? I think as far as makedocs/doctest goes, the behavior is exactly the same, no? Is there any difference with how a @warn/@error interacts with the strict keyword argument in makedocs (warnonly in Documenter 0.28/1.0)?

To get to the core of my question:

@Sbozzolo:

The reason I was suggesting moving to warning [is that] the execution continued after the error

That's a personal preference, though, right? – And I'm not sure I agree. I've been using the following mental model:

  • error(msg): Fatal error, execution cannot continue
  • @warn msg: There's something that needs the user's attention (but it might be ok)
  • @error msg: There's something wrong. We can keep running, but the user is definitely not getting the result they're expecting.

So in some sense @error is just a @warn with more emphasis ("you can't just ignore this"), but not necessarily a fatal error. In DocumenterCitations, I've been considering, e.g., missing citation references (whether due to forgetting to specify a .bib file or due to citing a non-existing keys) as errors, while, e.g., malformed bibtex entries are warnings.

One reason I often use non-fatal @error messages: It can be useful to get multiple errors (e.g., multiple missing references), so that the user can fix all of the underlying issues before rerunning the program. With fatal errors, you often get a tedius "run, error, fix" loop.

It would be interesting to know if there's a general consensus in the community on whether @error messages are expected to stop execution.

Thanks for sharing your thoughts about this. I find your mental model quite useful. I didn't think about the option of "error as a log message", and I now see the rationale behind it.

@goerz
Copy link
Member

goerz commented Sep 11, 2023

Alright, I think I'll keep it an @error then, but I'll look into mitigating this in some other way

@goerz goerz added help wanted Extra attention is needed enhancement New feature or request and removed help wanted Extra attention is needed labels Sep 13, 2023
@goerz
Copy link
Member

goerz commented Sep 28, 2023

The correct solution is to just skip all the citation pipeline steps when running in doctest mode by calling, e.g.,

Documenter.is_doctest_only(doc, "CollectCitations") && return

in Selectors.runner(::Type{CollectCitations}, doc).

I'll make sure to do that in a future release.

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

Successfully merging a pull request may close this issue.

2 participants