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 docstring for TOML stdlib module #52834

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

jam-khan
Copy link
Contributor

This is a PR for the issue #52725.

Thanks!

base/toml_parser.jl Outdated Show resolved Hide resolved
stevengj
stevengj previously approved these changes Jan 10, 2024
@stevengj stevengj added domain:docs This change adds or pertains to documentation stdlib Julia's standard library status:merge me PR is reviewed. Merge when all tests are passing labels Jan 10, 2024
@stevengj
Copy link
Member

Looks good to me, thanks for contributing!

@fredrikekre
Copy link
Member

This isn't TOML.jl though, just the Base internal TOML parser. The docstring should discourage use of Base.TOML and point to the TOML.jl standard library.

@stevengj stevengj dismissed their stale review January 13, 2024 14:06

whoops wrong place

@stevengj
Copy link
Member

stevengj commented Jan 13, 2024

Whoops, right, I didn't notice that the docstring was in the wrong place. Base.ispublic(Base, :TOML) == false, so this internal module doesn't need a docstring.

Just move this docstring to stdlib/TOML/src/TOML.jl.

(It wouldn't hurt to add a docstring to base/toml_parser.jl along the lines of what @fredrikekre suggested, too, e.g. "`Base.TOML` is an internal, undocumented TOML parser; users should call the TOML.jl standard library instead.")

@jam-khan
Copy link
Contributor Author

jam-khan commented Jan 13, 2024

Hi, Thanks for pointing it out @fredrikekre. I moved the docstring to TOML.jl and added another docstring in toml_parser.jl to discourage its use.

base/toml_parser.jl Outdated Show resolved Hide resolved
Improved Docstring

Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
stevengj
stevengj previously approved these changes Jan 13, 2024
stdlib/TOML/src/TOML.jl Outdated Show resolved Hide resolved
@stevengj stevengj removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 13, 2024
@stevengj stevengj dismissed their stale review January 13, 2024 17:54

need to fix blank line

@stevengj
Copy link
Member

Now that #52723 is merged, you'll need to update

@testset "Docstrings" begin
undoc = Docs.undocumented_names(TOML)
@test_broken isempty(undoc)
@test undoc == [:TOML]
end
to

@testset "Docstrings" begin
    @test isempty(Docs.undocumented_names(TOML))
end

@jam-khan
Copy link
Contributor Author

Hi, I have updated the runtests.jl. Thanks!

@stevengj stevengj added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 15, 2024
@stevengj
Copy link
Member

LGTM, thanks!

@LilithHafner LilithHafner changed the title Added docstring for TOML module (#52725) Add docstring for TOML stdlib module Jan 15, 2024
@LilithHafner LilithHafner merged commit 89710bf into JuliaLang:master Jan 15, 2024
6 of 9 checks passed
@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants