Replace isexported with ispublic to filter names in @autodocs

[gdalle]

Partial fix for #1506

I don't know how to test this because it depends on the Julia version. I tried to do something like

@static if isdefined(Base, :ispublic)
    public f
else
    export f
end

in the test module AutoDocs.Pages.E but Julia wouldn't parse it.

I also haven't added any badge to docstrings of private symbols. I think it might require more advanced modifications of e.g. the DocsNode or Node struct. This struct could actually incorportate two bits of info: whether the symbol is exported, and whether it is public.

[asinghvi17]

Thanks! I'm happy to approve this PR, but if you'd like to add a test I think Compat.jl has an @public macro you can use. public x won't parse in Julia < 1.11.

[gdalle]

I think this is good to go

[gdalle]

However I think this could break some workflows by including docstrings which are documented elsewhere, triggering a "duplicate docstrings" error. How does Documenter handle SemVer for this sort of thing?

[asinghvi17]

docstrings which are documented elsewhere

Could you expand a bit on what you mean here? I don't think there would be much of a difference between public and exported at this stage, since you need 1.11 compatibility for public anyway...

IIRC we have a canonical=false keyword to docs blocks for this reason, to establish which location is the 'canonical reference' that all @refs point to.

[mortenpi]

However I think this could break some workflows by including docstrings which are documented elsewhere, triggering a "duplicate docstrings" error. How does Documenter handle SemVer for this sort of thing?

Yes, this is my main concern right now. I think this creates a behavior change if you have a package that already uses the public keyword? And public is in 1.11, so it's very much out in the wild by now. That feels like a breaking change.

Since Documenter is normally auto-upgraded, the policy is to be conservative and not introduce breaking changes of that sort.

The easy solution is to guard this with an option to makedocs, but I wonder if there's something I'm missing, and there's some better solution?

[asinghvi17]

We could make public-but-not-exported docstrings non canonical by default? That would at least not be breaking, and maybe we could also issue a warning that contains all public-but-not-exported docstrings that have been documented elsewhere.

This might also need a change to the check on whether all exported docstrings are documented - maybe we need to expand that criterion to all public or exported docstrings.

[gdalle]

This might also need a change to the check on whether all exported docstrings are documented - maybe we need to expand that criterion to all public or exported docstrings.

That would be breaking for the same kind of reason I guess.

[gdalle]

We could make public-but-not-exported docstrings non canonical by default? That would at least not be breaking, and maybe we could also issue a warning that contains all public-but-not-exported docstrings that have been documented elsewhere.

Would that really be non-breaking? It may leave some symbols without any canonical docstring, whereas they had one before. Does that mean the links are dangling?

[mortenpi]

This might also need a change to the check on whether all exported docstrings are documented - maybe we need to expand that criterion to all public or exported docstrings.

We should be consistent, yes.. but it depends a bit on how we're phrasing things right now.

We could make public-but-not-exported docstrings non canonical by default?

I think this will become confusing to users down the line.

Thinking a bit more -- maybe we can be okay with it, if we add a big warning in the CHANGELOG? It would be a shame not to have ispublic the default behavior. So, not supporting public could be considered a bug. Ideally we would have done this change during the 1.11 development cycle already.

Would it be feasible to check that, if a duplicate docstring error gets detected, we check if it's due to the "public-but-not-exported" case, and print a warning with the context? We'd might still break existing workflows, but we can immediately tell the user what's going on.

We should still do the change in a minor release though, due to it's slightly "technically breaking change" character.

[mortenpi]

This might also need a change to the check on whether all exported docstrings are documented

Looking at checkdocs -- we default to :all (which should include non-public ones), and then we have :exported for exports. I think we just add :public there, to check for all ispublic docstrings. So I think we're fine here w.r.t. to any breakage.

[gdalle]

Would it be feasible to check that, if a duplicate docstring error gets detected, we check if it's due to the "public-but-not-exported" case, and print a warning with the context? We'd might still break existing workflows, but we can immediately tell the user what's going on.

Done in the latest commit.

Looking at checkdocs -- we default to :all (which should include non-public ones), and then we have :exported for exports. I think we just add :public there, to check for all ispublic docstrings. So I think we're fine here w.r.t. to any breakage.

Done in the latest commit.

---

Do we need additional tests for these two changes? I think we would want at least one for the new warning, otherwise that line won't get hit. Where should it go?

[mortenpi]

And thinking even more: I think giving context on the warning is a good idea also because you can run into this scenario:

• You have a package with no public markings, and you run doc builds on e.g. 1.10.
• You use Compat.jl to add public marking, but keep running doc builds on 1.10 for now.
• You update the Julia version for the doc build, and you get errors / a behavior change.

But this is fine too, since the "problem" is actually the user declaring things public conditionally on the Julia version. So we can argue that it's not Documenter's fault. But we can still hopefully provide a useful error message.

---

All in all, I think I'm warming up to doing the "breaking change" here. But let's leave this open for a week or so for feedback. And let me also solicit opinions from @fredrikekre, @goerz and @odow.

[mortenpi]

Do we need additional tests for these two changes? I think we would want at least one for the new warning, otherwise that line won't get hit. Where should it go?

Maybe here? https://github.com/JuliaDocs/Documenter.jl/blob/master/test/errors/make.jl

[gdalle]

I think there's another place where the warning should be included in case of duplicate docstrings, namely here: https://github.com/gdalle/Documenter.jl/blob/580be25b7b0f358674770909e799da270cf26308/src/expander_pipeline.jl#L421-L433

However I can't seem to get the module in which to test whether names are public or private. Can someone help?