Log @error for any DocCheck which triggers a failure.

[simonbyrne]

This makes it easier to distinguish other warnings (e.g. redirects) from those which actually trigger a DocCheck failure.

Fixes #1088.

Edit by @mortenpi: changed the issue number that this will close.

[mortenpi]

Ah! These should actually depend on whether strict = true or not, so they should use @logmsg. Relevant issue is #1088.

[simonbyrne]

Ah! These should actually depend on whether strict = true or not, so they should use @logmsg. Relevant issue is #1088.

Is that worth it? @error doesn't trigger a failure, it's just a higher-level log message.

[mortenpi]

I think so. It's gonna obscure real errors when you're running strict = false and something is still crashing the build.

[simonbyrne]

It's just as confusing at the moment: trying to figure out why strict=true is extremely difficult, because of #1344 (comment).

I would argue strict=true should throw a real error immediately (as then you get a stack trace), rather than logging them all and trying to figure out where the issues is.

[mortenpi]

The rule of thumb I would go for is that it's red if and only if it's gonna crash the build. If you're running strict=false, then e.g. linkcheck or missing docstring warnings are advisories.

I completely agree that Documenter's errors/warnings are not the most user-friendly. However, I am not sure a stacktrace deep into to Documenter's internals is going to help the user that much either. A better solution is to improve the error messages.

Regarding whether to error one by one or all at once: when you're working on docs, you often end up with many errors. So I think it's actually better user experience to show all the errors at once, rather than having the user re-build the docs over and over again, as they're tracking down all the issues. What I imagine is that you see a ton of red in the terminal, you go through them one by one, fix the issues, and then when you re-run the build, it should pass.

[simonbyrne]

However, I am not sure a stacktrace deep into to Documenter's internals is going to help the user that much either.

I disagree: if I can see where the error is triggered, I can find out what I need to fix it. Currently all I get is a bunch of "Warnings" with no indication which one is the problem.

[mortenpi]

Sorry, I missed your reply here.

if I can see where the error is triggered, I can find out what I need to fix it.

I genuinely do not see how knowing that there is an error() call on line 62 of Documenter.DocChecks.missingdocs helps you track down the actual problem (which, in this case, would be a missing line in an at-docs block in one of the Markdown files).

That said, if you believe it would be useful, we could have it as an option that you could enable (e.g. by passing strict = :immediate). However, I strongly believe that it should not be the default behavior -- on CI, you can't really (easily) keep building your docs over and over again. Instead, it's more handy if you get a full list of errors on CI.

However, either way, when Documenter should throw an error is orthogonal to how the error messages should be styled.

Currently all I get is a bunch of "Warnings" with no indication which one is the problem.

A thing should be a problem (i.e. cause Documenter to throw an error) if and only if it is an error. It is true that currently Documenter is not entirely consistent on that front (#1088), but dynamically picking the error level with @logmsg would solve that.

[simonbyrne]

Okay, I've updated the PR to vary the log level as you suggest in #1088. If you're happy with this approach, I can use it elsewhere.

It could be slightly simplified by using a macro, e.g.

macro logstrict(doc, label, args...)
    quote
        push!($(esc(doc)).internal.errors, $(esc(label)))
        @logmsg $(esc(doc)).user.strict ? Logging.Warn : Logging.Error $(args...)
    end
end

[simonbyrne]

bump?

[mortenpi]

Sorry for the delay. This looks great!

I think the macro approach would make it much cleaner, so if you don't mind updating the PR, it would be great to have. But we can also go with the current implementation.

[mortenpi]

I'll go ahead and merge this for inclusion in 0.25.1. We can always change to a macro later.