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

Variable docstring synchronization between command line help and manual #16328

Merged
merged 13 commits into from
May 13, 2016

Conversation

omus
Copy link
Member

@omus omus commented May 12, 2016

Uses new Sphinx directive "variable" to synchronize docstrings for variables with the manual. I created several new docstrings based upon the descriptions in the manual.

Requires JuliaLang/JuliaDoc#22 (merged)
Partially addresses #16303.

Uses new "variable" directive to synchronize variable docstrings to the
manual. Added docstrings for several constants.
@omus
Copy link
Member Author

omus commented May 12, 2016

Currently nothing, ANY, JULIA_HOME, and CPU_CORES are in the helpdb file. I should be able to get JULIA_HOME and CPU_CORES out of helpdb. I'm not sure where nothing or ANY should go as they aren't explicitly defined in base. Suggestions are welcome.

@omus
Copy link
Member Author

omus commented May 12, 2016

cc @MichaelHatherly: I've modified genstdlib.jl

@omus omus changed the title WIP: Improved docstring synchronization between command line help and manual WIP: Variable docstring synchronization between command line help and manual May 12, 2016
@MichaelHatherly
Copy link
Member

in the helpdb file

Perhaps in basedocs.jl? That's where other docstrings without declarations have been going. Note that instead of :nothing etc. you can just put nothing. No need to quote the object, it should have the same effect.

"""
VERSION

An object describing which version of Julia is in use.
Copy link
Contributor

Choose a reason for hiding this comment

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

A VersionNumber object

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

further down it looks like you were just porting over the existing rst description, so consider this an enhancement :)

@omus omus force-pushed the variable-directive branch from adf3949 to 56d4580 Compare May 12, 2016 18:35
@omus
Copy link
Member Author

omus commented May 12, 2016

I have updated all the docstrings for constants/variables listed in stdlib/constants. Additionally I noticed most of the "See also" references were broken so I fixed that as well.

I'm sure I have missed some constants that should be updated but I think this PR has modified enough files ;)

@omus omus changed the title WIP: Variable docstring synchronization between command line help and manual Variable docstring synchronization between command line help and manual May 12, 2016
""" ->
$constant
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having 3 similar docstrings perhaps something like the following?

const MS_ASYNC = 1
const MS_INVALIDATE = 2
const MS_SYNC = 4

@doc """
    MS_ASYNC
    MS_INVALIDATE
    MS_SYNC

Enum constants for [`msync`](:func:`msync`). See your platform man page for details.
(not available on Windows).
""" ->
(MS_ASYNC, MS_INVALIDATE, MS_SYNC)

and then adjust the signature in the rst docs accordingly.

(@doc needed since this is inside a @unix_only block, not toplevel.)

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I get this to match up with the manual?

WARNING: duplicate signature found for 'Base.MS_INVALIDATE' in module 'Base':

    MS_ASYNC
    MS_SYNC
    MS_INVALIDATE

WARNING: duplicate signature found for 'Base.MS_ASYNC' in module 'Base':

    MS_ASYNC
    MS_SYNC
    MS_INVALIDATE

Copy link
Member

@MichaelHatherly MichaelHatherly May 12, 2016

Choose a reason for hiding this comment

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

Adding a

.. variable:: MS_ASYNC
              MS_INVALIDATE
              MS_SYNC

declaration should be sufficient I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems so obvious now... I still get the warnings however.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that's probably a bug in genstdlib, since the docstrings are the same object rather than different docstrings that happen to have the same signature. I'll try track that one down.

Copy link
Member

Choose a reason for hiding this comment

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

Could you try applying this diff and see if the duplicate warnings are gone?

diff --git a/doc/genstdlib.jl b/doc/genstdlib.jl
index b03a146..85e28fb 100644
--- a/doc/genstdlib.jl
+++ b/doc/genstdlib.jl
@@ -111,7 +111,7 @@ function loaddocs!(state::State, mod, binding::Binding, typesig, docstr)
     markdown = Base.Docs.parsedoc(docstr)
     if validdocstr(markdown)
         code = rstrip(markdown.content[1].code)
-        if haskey(state.validdocs, code)
+        if haskey(state.validdocs, code) && state.validdocs[code][3] !== docstr
             code = indent(code)
             warn("duplicate signature found for '$binding' in module '$mod':\n\n$code\n")
             state.errorlevel = 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Patch does fix the duplicate signature warnings. Should I include the patch as part of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do. I don't want to hold up your PR with different one line PR :)

@omus omus force-pushed the variable-directive branch from 52adaea to dbe2c3f Compare May 12, 2016 21:36
@tkelman tkelman merged commit 3624e08 into JuliaLang:master May 13, 2016
@omus omus deleted the variable-directive branch May 13, 2016 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants