-
-
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
Replace -> with :: where appropriate in docstrings #57012
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point I made a branch switching to -->
instead (but never made a PR). One of the goals was things like this, where what is returned seems more important than the type:
lmul!(a::Number, B::AbstractArray) --> B # mutates & returns 2nd arg
eigvals!(A, B; sortby) --> values::Vector # returns neither arg
I see the PR would make these eigvals!(A, B; sortby) -> values::Vector
. Perhaps that seems awkwardly almost-code (same complaint as the present ->
)? My idea was to make a smaller visual change, and move in the direction of clearly-not-syntax.
I also had these... which do parse as capturing :(s --> Bool)
but at least -->
has no definition, while s::Bool
does mean something which isn't true:
@isdefined s --> Bool
@__LINE__ --> Int
@@ -1443,7 +1443,7 @@ function _prepend!(a::Vector, ::IteratorSize, iter) | |||
end | |||
|
|||
""" | |||
resize!(a::Vector, n::Integer) -> Vector | |||
resize!(a::Vector, n::Integer) -> a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or switched ... to
ret_name::RetT
.
Took a while to find an example of what exactly this meant.
If the reason to use ::
is that this is valid syntax, does this want to use ->
when it's not?
@@ -3,7 +3,7 @@ | |||
# Conversion/Promotion | |||
|
|||
""" | |||
Date(dt::DateTime) -> Date | |||
Date(dt::DateTime)::Date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should cases like this be annotated at all, or is it just clutter?
The default assumption that SomeType(...)
must construct a SomeType
, exceptions (at there any?) need a giant warning. Annotating all the normal cases, if anything, seems to hint that there might be exceptions to watch out for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with you: Just clutter.
@@ -147,7 +147,7 @@ macro specialize(vars...) | |||
end | |||
|
|||
""" | |||
@isdefined s -> Bool | |||
@isdefined s::Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parses as calling the macro on :(s::Bool)
, which isn't great. (The old one parsed a a function of course, but the point of the PR is to remove that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I intended to skip macros in this PR but accidentally included that. Should be fixed (probably just reverted/left alone)
What about using assignment syntax to name variables instead? B = lmul!(a::Number, B::AbstractArray)
values = eigvals!(A, B; sortby)::Vector
maxval, index = findmax(A; dims) |
Previous discussion on this at #22804 |
Reading the previous discussion and existing docstrings, I propose that
|
I like the idea from @mbauman to use → instead of -> to distinguish it from anonymous functions. If this is used, I'd also prefer having it consistently, even if only the type is specified:
|
In cases where a function is documented as
I either switched
->
to::
or switchedRetT
toret_name::RetT
.From the recommendation and justification from @nsajko here: #56978 (comment) and applied throughout the repo.