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.

I think this is good to go

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?

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.

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?

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.

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.

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?

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.

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.

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?

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.

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

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?

@gdalle I took some liberties in re-arranging the code a bit as I was thinking what to do with the other error. If it looks good to you though, I think we can get this merged and tagged (in particular, it would be good to get 👀 on the expanded error message and CHANGELOG).

I think what we can do is just use the .mod field of the binding object there. I think there might be cases where that's not quite consistent, because ispublic for a function depends on the module. I.e. consider this:

julia> module Foo; import Base: cd; end
Main.Foo

julia> module Bar; import Base: cd; public cd; end
Main.Bar

julia> Base.ispublic(Base, :cd)
true

julia> Base.ispublic(Main, :cd)
false

julia> Base.ispublic(Foo, :cd)
false

julia> Base.ispublic(Bar, :cd)
true

And I am not off the top of my head 100% sure what this implies about how we even include docstrings. But since this is just about printing an informative error, I think we can live with any potential inconsistencies.

Note that codecov fails to run, so we're not sure the new code is hit during tests

Gentle bump on this @mortenpi

Thank you @gdalle! I think let's go and merge this!

but I wonder whether more tests should be added first.

I think we're okay, let's not postpone this. We can always add more tests later.. for example together with any bugfix PRs that might be necessary 😄

Awesome! I'm excited to bring the full power of the public keyword to the world 😎

What's the release process for Documenter? is it triggered automatically?

It's manual, but let's aim to get the release out this week. There are a few more PRs that are probably ready to merge, so we can bundle them all together in one release.

Hey @mortenpi, just a gentle bump on this. When do we finally get to break every Documenter workflow in the ecosystem ^^?

I know, sorry. There are a few PRs I'd hope we can get into the same release though: #2654

Sounds good!