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

Moving Docstrings #12703

Closed
wants to merge 4 commits into from
Closed

Moving Docstrings #12703

wants to merge 4 commits into from

Conversation

MikeInnes
Copy link
Member

Or, the final step in Julia's doc transition.

This PR begins the process of moving our shiny new markdown docstrings into Base, and should hopefully clarify the process of adding and modifying documentation as well. If you want to help out, read on!

In the first commit I've simply moved the Pkg docstrings from base/docs/helpdb.jl to their appropriate locations in the pkg.jl file. You'll notice that I've split functions with multiple signatures, such as dir(), and put each method next to its definition. (Isn't it beautiful?) These will be combined in the same order when viewed in the REPL. I also reflowed the paragraphs to 92 chars, which is helpful but not essential.

As with code changes, getting Julia to recognise doc changes requires a make. It's worth building and checking that things look right in the repl help at this stage.

I made a couple of tweaks to the Pkg docstrings, which means that the RST manual is now out of date. Easy enough to fix; in commit two I ran julia doc/genstdlib.jl (after make) and pkg.rst was updated appropriately.

In commit three I added a new docstring to a currently-undocumented function, Pkg.dependents. After a quick make this is available from the REPL with ?Pkg.dependents. Simples!

However, I also want to add this docstring to the manual. This is easy to do as well; I just added the line .. function:: dependents to the end of pkg.rst, and after another run of genstdlib.jl it appeared, as if by magic.

That's basically as much as you can want from documentation, I think. (Or at least, it's as much as I can think of right now. It's late.) If you're interested in helping out, please go ahead and pick some docstrings from helpdb.jl, and move them to appropriate places. The only caveat is that it's probably best to avoid docstrings emblazoned with `rst```` for now.

One of the main reasons we pushed for this transition was to really put the doc system through its paces and shake out any remaining issues before releasing it into the wild. If you have any issues or need any kind of assistance with it, please let us know!

@StefanKarpinski
Copy link
Member

💯 🎉 🍰

@MikeInnes MikeInnes mentioned this pull request Aug 19, 2015
dir(path...) = Dir.path(path...)

"""
init(meta = DEFAULT_META, branch = META_BRANCH)
Copy link
Contributor

Choose a reason for hiding this comment

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

worth keeping the type info here, isn't it?

@KristofferC
Copy link
Member

🍰!

My first question (out of probably many). Why does some strings start with doc and not others?

@MichaelHatherly
Copy link
Member

Why does some strings start with doc and not others?

Docstrings which need LaTeX $ syntax have to use doc"...", others don't need to though.

@IainNZ
Copy link
Member

IainNZ commented Aug 19, 2015

@IainNZ
Copy link
Member

IainNZ commented Aug 19, 2015

Is there any harm/cost to putting doc even if its not needed? I kind of like the explicitness of it.

@MichaelHatherly
Copy link
Member

Is there any harm/cost to putting doc even if its not needed? I kind of like the explicitness of it.

No, it shouldn't make a difference.

@prcastro
Copy link
Contributor

Very nice! I noticed that almost all docstrings contain the method signature. Would it be possible to include the method signature using a command (like %signature%, or something like this)? For types, this would include all inner constructors on the docstring.

@MichaelHatherly
Copy link
Member

Would it be possible to include the method signature

For the moment that's going to need to be done manually. Would be good to eventually do it automatically, but probably not before 0.4 unless @one-more-minute has something planned already.

@ScottPJones
Copy link
Contributor

Why can't the method signature be picked up with @which later, instead of having to copy it manually into the doc string, or have some marker like %signature%?
I noticed a problem (which @tkelman also commented on), that all the documentation I added for UTF convert functions ended up in the documentation for generic convert.
It would maybe be nice, if you ask for the documentation for a name, and there is documentation attached to the generic function, to just return that, and not all the documentation for all of the specific methods. If you ask for a specific method, and there is documentation for it, I'd love to see always the signature, followed by the specific documentation. (If there is no documentation specifically for it, but there is for the generic function, then I'd like to see first the signature, followed by the generic documentation, if any).
Would any of that be possible, or am I way off base?

@MichaelHatherly
Copy link
Member

functions ended up in the documentation for generic convert

They're not actually in the docs for the convert Function, it's just the way they're prepared for displaying here. They're still being stored by Method so it shouldn't be too difficult to only display docs for just a Function or just a Method. That's kind of how Docile has been displaying docs and worked quite nicely I thought.

@ScottPJones
Copy link
Contributor

I explicitly used the new syntax for defining a generic function, i.e. function name end for the documentation I wrote for Base.checkstring and Base.unsafe_checkstring (note: unexported but documented functions!).

@hayd
Copy link
Member

hayd commented Aug 20, 2015

This issue is missing a movie-based title.


Why can't the method signature be picked up

I agree with that. (In the future, not right now though) see also #9838 (and your #12437).

@ScottPJones
Copy link
Contributor

This issue is missing a movie-based title.

Yes, this PR should definitely not be merged until that item is taken care of! 😜

@MikeInnes
Copy link
Member Author

This issue is missing a movie-based title.

Yeah, all I had left was "The Docshank Redemption" and I was too ashamed.

RE discussion of methods: We're yet to settle on a lot of documentation conventions, but one thing I'm very strongly in favour of is always treating docs for a function as a single unit, not as a collection of independent methods.

push!(xs, x) is a great example of a function that's really coherent. It may have 15 methods, but it always does the same thing – putting x into xs – so you only need one docstring for the function. If it did need fifteen docstrings, that would be a serious code smell, because it means that implementation details are leaking. In general, methods are an implementation detail that the user of a function shouldn't have to worry about. Heisenfunctions that change behaviour depending on their input types are something we should strongly discourage.

There are a couple of really good reasons to split a docstring across methods. Occasionally you have different APIs depending on arity (unlike dependency on types this is generally OK) and want to make the source clearer, as in the current package docs. The other is to add important notes for particular types (without changing the fundamental behaviour of the function), e.g.:

""""
    x*y
Multiply `x` and `y` together.
"""
*(x, y) = ...

"When applied multidimensional arrays, matrix (as opposed to element-wise) multiplication
is used."
*(::AbstractArray, ::AbstractArray) = ...

The key point to notice in both cases is that this is still written as a single docstring for a single function, it just happens to be spread out a little.

On top of being good code style, I think it makes a lot of sense from a user perspective as well. If I'm using push! I want to very quickly type ?push! in the repl and get enough information to use it – what it does, order of arguments – rather than digging around to find specific methods. The current docstrings from the manual gets this really right and I think future docstrings should follow suit. Having N different docstrings for a given function is going to make it really tricky to work out what should be displayed where, so this simplifies that a lot.

@MichaelHatherly I know you've been approaching things slightly differently to this in Docile, so I'm interested in your take – let me know if I'm missing a crucial concern here.

@MichaelHatherly
Copy link
Member

let me know if I'm missing a crucial concern here.

No, that was all really well-reasoned I think.

Yeah, all I had left was "The Docshank Redemption" and I was too ashamed.

lol

@quinnj
Copy link
Member

quinnj commented Aug 20, 2015

My important and necessary input:

  • The Docfather
  • Donnie Doc-ko
  • Raiders of the Lost Doc
  • Dude, Where's my Docstring?

@waldyrious
Copy link
Contributor

  • Docma
  • Slumdoc Millionaire
  • Docs of Hazzard
  • Doc Day Afternoon

@hayd
Copy link
Member

hayd commented Aug 20, 2015

Keeping docstrings general seems very worthwhile, I like that, especially for methods which are frequently extended. We could definitely do a better job in Base.

For packages it's questions like "should I document what multiplication, *, means in this context"?

Pretty much every time I use help follow it up with methods(..), so I want to see that prepended to help. :)

@vtjnash
Copy link
Member

vtjnash commented Aug 21, 2015

can we also get an item added to NEWS.md explaining where we should be putting docs now (and how to transition them)

@ScottPJones
Copy link
Contributor

Given that this PR seems to be about figuring out where docstrings have moved to, I second @quinnj's suggestion of renaming this "Dude, where's my Docstring?" (it's called truth in advertising) 😀

@StefanKarpinski
Copy link
Member

+1 for "Dude, where's my Docstring?"

@catawbasam
Copy link
Contributor

Is anything blocking this from merging?
Also, it could use 'doc' and 'docsystem' tags.

@pao pao added docs This change adds or pertains to documentation docsystem The documentation building system labels Aug 30, 2015
@jakebolewski
Copy link
Member

I would strongly suggest not moving docstrings until #12573 is sorted out. Adding indirection to the current process complicates things.

@artkuo
Copy link
Contributor

artkuo commented Oct 22, 2015

Is there a policy or guidance on ordering of docstrings vs. ordering of methods? Users probably prefer to have the docstrings be ordered from general application to more specific. For example, in 0.4, ?cor says it computes the Pearson correlation, as one would hope. With docstrings moving to code, the ordering usually goes with the code, often specific to general. So in 0.5.0-dev, the first thing ?cor returns is the rather alarming

cor(x)
Return the number one.

This is because cor has a special case for the (rare) instance of a vector x input. It makes sense to code from specific to general, because the programmer wants to think in terms of the dispatch mechanism, and it is probably easier to catch bugs this way. But this is just confusing to the user.

I believe the user should be prioritized in help. One solution is to have docstrings appear in the order most helpful to user, prior to a group of related method definitions, which can be in their coder-preferred order. Another would be to actually order them like the help.

Is there a policy for docstring order, and should there be one?

@MichaelHatherly
Copy link
Member

@artkuo, docs for methods are sorted by signature using type_morespecific here when they're first added to a module's __META__. Then when displaying results the order is reversed here. So the order in which they're defined shouldn't be making any difference to the order they are displayed in. If that isn't happening somewhere then it's a bug.

I agree that in some cases the ordering isn't that great and there probably are better ways to sort them. This PR isn't really the right place to discuss that though. Could you open a new issue? Thanks!

@artkuo
Copy link
Contributor

artkuo commented Oct 23, 2015

Thanks for the explanation @MichaelHatherly. I'll try to see what's causing that particular docstring to come out in apparently wrong order, and file an issue if it is one.

@MikeInnes MikeInnes closed this Sep 7, 2016
@tkelman tkelman deleted the omm/doc-tutorial branch September 7, 2016 12:23
@waldyrious
Copy link
Contributor

waldyrious commented Sep 7, 2016

@MikeInnes, @tkelman, was this done in another PR? It would be nice to have them linked :) a brief look at this list indicates #17959 and #18041 as potential candidates, but there way be more.

@tkelman
Copy link
Contributor

tkelman commented Sep 7, 2016

look at the commit history of the helpdb folder, or blame on the files with the now inline docstrings. I'd be amazed if these hadn't been moved by now.

@kshyatt
Copy link
Contributor

kshyatt commented Sep 7, 2016

There are more things that can be moved out of HelpDB. I stopped where I did because the number of comments on that #18041 was really large and poor @tkelman had to take like 5 at-bats to review it. In particular I think the file/regex stuff can go out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation docsystem The documentation building system
Projects
None yet
Development

Successfully merging this pull request may close these issues.