Skip to content

Replace isexported with ispublic to filter names in @autodocs #2629

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

Merged
merged 19 commits into from
Mar 5, 2025

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Feb 11, 2025

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.

@gdalle gdalle changed the title Replace isexported with ispublic as criterion for @autodocs Replace isexported with ispublic to filter names in @autodocs Feb 11, 2025
Co-authored-by: Anshul Singhvi <[email protected]>
@asinghvi17
Copy link
Collaborator

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 and others added 2 commits February 11, 2025 23:19
@gdalle gdalle requested a review from asinghvi17 February 11, 2025 22:34
@gdalle
Copy link
Contributor Author

gdalle commented Feb 11, 2025

I think this is good to go

@gdalle
Copy link
Contributor Author

gdalle commented Feb 11, 2025

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Contributor Author

gdalle commented Feb 11, 2025

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
Copy link
Contributor Author

gdalle commented Feb 11, 2025

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
Copy link
Member

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
Copy link
Member

mortenpi commented Feb 11, 2025

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
Copy link
Contributor Author

gdalle commented Feb 11, 2025

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

gdalle commented Feb 20, 2025

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?

@mortenpi
Copy link
Member

mortenpi commented Feb 27, 2025

@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.

Copy link
Contributor Author

@gdalle gdalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactor looks good to me, I made a few suggestions on error messages. I'm okay with you merging this, but I wonder whether more tests should be added first. If so, please feel free to take over and add them wherever you feel they would be relevant.

@gdalle
Copy link
Contributor Author

gdalle commented Feb 27, 2025

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

@gdalle
Copy link
Contributor Author

gdalle commented Mar 3, 2025

Gentle bump on this @mortenpi

@mortenpi
Copy link
Member

mortenpi commented Mar 5, 2025

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 😄

@mortenpi mortenpi enabled auto-merge (squash) March 5, 2025 07:41
@gdalle
Copy link
Contributor Author

gdalle commented Mar 5, 2025

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

@mortenpi mortenpi merged commit 09eb5a5 into JuliaDocs:master Mar 5, 2025
21 checks passed
@gdalle gdalle deleted the gd/public branch March 5, 2025 08:05
@gdalle
Copy link
Contributor Author

gdalle commented Mar 5, 2025

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

@mortenpi
Copy link
Member

mortenpi commented Mar 5, 2025

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.

@gdalle
Copy link
Contributor Author

gdalle commented Mar 11, 2025

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

@mortenpi
Copy link
Member

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

@gdalle
Copy link
Contributor Author

gdalle commented Mar 11, 2025

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants