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

Add a way for users to tell Documenter about unexported names that are part of their package's public API #1507

Conversation

DilumAluthge
Copy link
Contributor

Fixes #1506

This pull request adds the ability for users to tell Documenter about unexported names that are part of their package's public API.

When using @autodocs, these names will be shown when Public = true, and will not be shown when Public = false.

In order to use this feature, simple define a vector of symbols named NONEXPORTED_PUBLIC_NAMES at the top level of your module. (We can definitely bikeshed the name NONEXPORTED_PUBLIC_NAMES .)

Here's an example:

module MyPackage

export a

const NONEXPORTED_PUBLIC_NAMES = Symbol[:b, :c]

"""
`a` is exported, and it is part of the public API.
"""
function a end

"""
`b` is not exported, but it is part of the public API.
"""
function b end
"""

`c` is not exported, but it is part of the public API.
"""
function c end

"""
`d` is not exported, and it is private (internal).
"""
function d end

end # module

In this example, Documenter will consider the names a, b, and c to be public (even though b and c are not exported). d will be considered to be private.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This looks good, I like this. Sorry for not reviewing this earlier.

  1. I think we should also update to using the new name_is_public in document checks:

    isexported = Base.isexported(mod, name)

  2. I was wondering whether "unexported" sounds better in the variable names. But FWIW, it does look like "non-exported" is more commonly used.

  3. Needs a rebase, but that should not be problematic (just .travis.yml has been removed in the meanwhile).

@@ -76,3 +76,5 @@ include("TestUtilities.jl"); using .TestUtilities
@info "doctest() Documenter's manual"
@quietly include("manual.jl")
end

include("public_names.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Could we include this in the main testset?

@mortenpi mortenpi added this to the 0.27.0 milestone Feb 18, 2021
Comment on lines +164 to +183
"""
`a` is exported, and it is part of the public API.
"""
function a end

"""
`b` is not exported, but it is part of the public API.
"""
function b end

"""
`c` is not exported, but it is part of the public API.
"""
function c end

"""
`d` is not exported, and it is private (internal).
"""
function d end
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it might be worth turning these into 1-line docstrings to save a few lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! What is the syntax for a one line docstring?

@MichaelHatherly
Copy link
Member

I agree that we definitely could do with this feature, I guess my question would be whether this is the right "level" to be adding it in, rather than further down in the docsystem, either within Base.Docs or in DocStringExtensions, since it's useful information for packages aside from Documenter.

The following implementation with DocStringExtensions works pretty well for tagging whether a DocStr is public on not:

struct Public <: Abbreviation end

const PUBLIC = Public()
export PUBLIC

format(::Public, buf, doc) = nothing

ispublic(doc::Docs.DocStr) = any(x -> isa(x, Public), doc.text)
julia> """
       $PUBLIC

       This is public.
       """
       f(x) = x
f

julia> """
       This is not public.
       """
       f(x, y) = x
f

julia> DocStringExtensions.ispublic.(@doc(f).meta[:results])
2-element BitVector:
 1
 0

Which has the benefit of working on the docstring rather than the binding since it could be useful to only make particular signatures of a binding public rather than all of them.

@fredrikekre
Copy link
Member

In order to use this feature, simple define a vector of symbols named NONEXPORTED_PUBLIC_NAMES at the top level of your module. (We can definitely bikeshed the name NONEXPORTED_PUBLIC_NAMES .)

If it will only be used by Documenter, wouldn't it be better to have this as a kwarg to makedocs?

@mortenpi
Copy link
Member

The NONEXPORTED_PUBLIC_NAMES variable would be available to other tools as well, so at least I don't see this being Documenter-only. It's more about establishing a convention for how to set, store and access this information.

A few thoughts:

  • In principle, having this at the Base.Docs level would be best, but that has issues with backwards compatibility.
  • Having DocStringExtensions as a dependency just to mark function public/non-public seems a bit heavy, although it would fit semantically. We could document a convention and DocStringExtensions could provide some syntactic sugar?
  • Alternatively, the API to declare things public/non-public could live in its own separate package? Not to encourage the proliferation of tiny packages, but in this case it might be beneficial, since that dependency would likely be extremely stable. A bigger a package is, the more likely it is that you start running into compatibility issues due to unrelated changes (the reason I am slightly skeptical of having the whole feature depend on DocStringExtensions).

@DilumAluthge
Copy link
Contributor Author

The NONEXPORTED_PUBLIC_NAMES variable would be available to other tools as well, so at least I don't see this being Documenter-only. It's more about establishing a convention for how to set, store and access this information.

Yeah exactly. Eventually I would like to have some kind of ecosystem-wide convention.

In Julia 2.0, I'd like to maybe have a keyword public built into the language for declaring nonexported public names. But that's a conversation for another day.

@DilumAluthge
Copy link
Contributor Author

DilumAluthge commented Apr 7, 2021

I'd like to avoid making package authors take on an additional dependency just so that they can use this convention.

Something in Base Julia (e.g. Base.Docs) is fine. But I don't want to require dependency on an external package.

@DilumAluthge
Copy link
Contributor Author

Which has the benefit of working on the docstring rather than the binding since it could be useful to only make particular signatures of a binding public rather than all of them.

I think it is sufficient to just work on the name, rather than specific docstrings. Basically, we are trying to emulate the functionality of export, and export works on the entire name.

@MichaelHatherly
Copy link
Member

The DocStringExtensions example was just illustrative of an alternative approach to this one, not a suggestion of an exact implementation or where it should live.

I still disagree with relying on convention and globals with magic names like this does. Why not implement an API for getting/setting the .data field of Base.Docs.DocStr within Base.Docs and then using Compat.jl for backwards compatibility for older versions of Julia? That would be a lot more general than a per-module global for just setting the visibility of bindings, but could still be used for that purpose as well.

@mortenpi
Copy link
Member

Why not implement an API for getting/setting the .data field of Base.Docs.DocStr within Base.Docs and then using Compat.jl for backwards compatibility for older versions of Julia?

Honestly, I forgot about Compat. That would indeed be a way to get around the compatibility issue, which is my only objection to implementing it in Base.

@DilumAluthge DilumAluthge deleted the dpa/non-exported-public-names branch May 23, 2021 16:09
@DilumAluthge
Copy link
Contributor Author

I'm not familiar enough with the Base.Docs code base to be able to work on implementing it there, at least not right now.

I do think we need this feature, and not just for Documenter, but in a way that all tools can use.

@MichaelHatherly
Copy link
Member

I don't have a lot of time at the moment to directly work one this, but can try provide some breadcrumbs at least to an initial implementation if need be. Something like the following as a MVP could get us partway there:

julia> macro metadata!(expr, data)
           :(merge!(Docs.@ref($(esc(expr))).data, $(esc(data))))
       end
@metadata! (macro with 1 method)

julia> macro metadata(expr)
           :(Docs.@ref($(esc(expr))).data)
       end
@metadata (macro with 1 method)

julia> @metadata sin(::Number)
Dict{Symbol, Any} with 5 entries:
  :typesig    => Tuple{Number}
  :module     => Base.Math
  :linenumber => 432
  :binding    => Base.sin
  :path       => "math.jl"

julia> @metadata! sin(::Number) Dict(:public => true)
Dict{Symbol, Any} with 6 entries:
  :typesig    => Tuple{Number}
  :module     => Base.Math
  :linenumber => 432
  :binding    => Base.sin
  :path       => "math.jl"
  :public     => true

A function-based API for retrieval (metadata(f, typesig)) of .data could be useful in the context of Documenter as well. As well as simpler syntax for setting key/value pairs instead of the verbose Dict literals, i.e. @metadata! sin(::Number) public = true.

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.

Tell Documenter about non-exported names that are part of my package's public API
4 participants