-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix #11798 & load bootstrapped docs. #11932
Fix #11798 & load bootstrapped docs. #11932
Conversation
0a59c80
to
3525d35
Compare
@one-more-minute this works for the original case in #11798 and a selection of random methods from base. If this is indeed a suitable fix then I'll add some tests and let it run on travis. |
3525d35
to
1060bfe
Compare
This seems like a reasonable approach, but as an alternative I'm wondering whether there's a way to check if the binding has been reified yet – i.e., check whether the We'll probably need something like this anyway, it's a bit fiddly and there are a lot of edge cases to go over. |
Yeah, that would be a lot cleaner. I'll resolve the conflicts here in any case. |
Does this work for parametrized functions and would it be possible to extend it (doesn't have to be in this PR) so that it's possible to document a method after it's definition? Either by documenting on the signature of a method as a I've also added document about only possible to document the whole function after it's defined so you might want to change that if necessary. |
1060bfe
to
9003144
Compare
It should do, using a
Syntax I used in Docile for that was "..."
(f, Foo, Bar) to document "..."
:(f(::Foo, ::Bar)) That should be a relatively simple addition to this PR if we do happen to go this route. |
Yeah, that's more or less what I'm thinking about and also documenting the Use case here #12000 (comment) |
And the reason I asked about parametrized method is that I'm wondering if you could hit some bug for comparison involving bound |
@MichaelHatherly We can't use the current trick for docs created during bootstrap anyway, so if you get this working solidly I'm happy to switch (even if it's a little less pretty). |
and
Directly in the module's
So there's no easy way to view docs for a documented
I've not run into that yet, thanks for the heads up. Regarding not constructing |
Ah... good point. I think this should be workable with a bit more testing (and an "ok" regarding constructing the |
Just to clarify that comment: I'm aware that |
For documenting methods after they're defined, would it be OK to just have – "..."
f(::Foo, ::Bar) ? We could also support "..."
@which f(1) but honestly, I think that's a bit confusing. @MichaelHatherly Is there any chance you'd be able to look at applying this work to the docstrings stored at bootstrap? Pre-bootstrap we only need to support a very limited subset of what the full doc system supports, e.g. just functions and methods, so it'd be great to push that forward here. |
Yeah, I guess until someone actually needs it (which would probably be quite unlikely outside of
Presumably all that needs to be done is something like: for (mod, doc, def) in Base.DocBootstrap.docs
eval(mod, :(Base.@doc($(doc), $(def))))
end Doing that a the REPL gave me the following error:
which is from julia/base/markdown/Julia/Julia.jl Lines 3 to 10 in e66175b
Other than that error the rest of the docstrings that are currently written inline look like they're in |
9003144
to
002cea8
Compare
It would be significantly less confusing if |
Commit two adds a first try at loading bootstrapped docs stored in At the moment the code is a little repetitive and I'll try to simplify it if this approach is suitable. |
9bf49b9
to
deb537c
Compare
Ok, I like the Loading bootstrap docs can happen as soon as the doc system is loaded, so it might be better to have it just after the |
deb537c
to
dac23b7
Compare
Done. |
dac23b7
to
8bba4b7
Compare
8bba4b7
to
d548339
Compare
d548339
to
15cac9b
Compare
Not sure what's up with appveyor, rebased and pushed again to hopefully fix that. |
Fantastic, this will really streamline the process of moving docs into Base. |
Fix #11798 & load bootstrapped docs.
Great, thanks. @one-more-minute, do we also want the syntax for adding docs to methods after definition similar to the way macros can be? As in: f(x::Int, y) = ...
"..."
:(f(::Int, ::Any)) I've got some working code for that somewhere. |
That would be great, but is there a reason it needs to be quoted? I think it's ok to disallow documenting the result of a function call. |
I'm fine with having it unquoted, was just following the macro version. |
I'm thinking the quote version can be easily wrapped in a macro (i.e. a custom macro that define a method can just evaluate to the signature expression of the function) and the i.e. would be nice if something like this can work macro def_f()
quote
$(esc(:(f(a::B, c::D, e::F...) = 1)))
# many random code
:(f(a::B, c::D, e::F...))
end
end
"""
some doc for `f`
"""
@def_f |
Ok, so you're imagining that if someone tries to document the value |
Right. Another possibility is to allow documenting the
Agree. I just imagine that most of the time people need to document a method after its definition is because the function is defined in a macro. Would be nice if the macro auther can handle this such that it can be documented just as a normal function definition. |
In general we're moving away from allowing values to be documented via I also think that documenting a value should always just do that. If you special case certain kinds of expressions, for example, then doc!(:(2+2), md"foo") and doc!(:(f()), md"foo") have totally different semantics, which strikes me as a recipe for confusion. I see your point and it would be good to have a solution to that problem, though. |
Personally I don't need this so I don't have much detail about the general use case. Ping @mauro3 since he is the first who brought this up in #12000 (comment) |
Since
A very contrived example, but the general idea is for each macro the author would define an object, here |
I love this idea! I didn't check how exactly would you construct |
You can put anything as the value, so it doesn't have to be a If this way works for those who need it, then clean up and documenting it would be good. |
I mean a helper would be nice so that people don't need to write their own one. (Although that's certainly not fundamental). |
It's a nice idea, but I'm not sure we should keep that fallback just for this feature. I'm concerned that as we add more syntax to It might be a lot simpler to just pass the docstring to the macro that defines the function. Here's another option: macro foo()
def = :(function f() end)
quote
$(Expr(:meta, :doc, def))
$def
# something fancy
end
end
"foo"
@foo
|
That seems reasonable as well. So long as whatever we choose is simple for people to hook into. From your example I'd say it's less of a hassle than having to define additional types just be able to hook into the docsystem and would probably provide better error messages. |
I've yet to see how well this works withinBase
and whether I've missed any cases, hence the use of[skip ci]
for now.Approach taken:
Extract the signature from a method definition and use
which
to determine theMethod
that has been defined. This avoids referencing theFunction
prior to defining it, which was the cause of #11798.