-
Notifications
You must be signed in to change notification settings - Fork 367
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 docstrings for names, size, and order #1814
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.
Thank you for looking into it. I will give general comments as they apply to all docstrings to improve their consistency (I know not all docstrings strictly follow these recommendations, but for the future we should try to be consistent):
- please use docstring formatting recommended in Base, i.e. first line should start with the signature, separate example with
### Examples
header - please make sure that you document all relevant types,
DataFrameRow
in particular - an appropriate entry to docs/src/lib/functions.md should be added for each documented function
src/abstractdataframe/sort.jl
Outdated
@@ -20,6 +20,14 @@ struct UserColOrdering{T<:ColumnIndex} | |||
end | |||
|
|||
# This is exported, and lets a user define orderings for a particular column | |||
""" | |||
User defined order for a particular column. Used in conjuction with sort!(df::DataFrame, cols). |
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.
Please quote code in the text. Also it would be better if example were runnable. Finally if a function accepts some arguments they should be documented.
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.
You are referring to the kwargs of order
, correct? Are there any besides rev
? I was a bit confused by the source.
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.
Any by "runnable", do you mean that it should avoid undefined variables?
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.
In the body of ordering(col_ord::UserColOrdering, lt::Function, by::Function, rev::Bool, order::Ordering)
you have a list of accepted kwargs.
By "runnable" I mean that when we enable docstring testing during CI (which we plan in the future) the code should run without error (as otherwise CI will fail). In this case it means that the use of undefined variable. The additional benefit of ensuring "runnability" is that the user can copy-paste the code and test what it does.
Thanks for the notes. I'm not sure what you're referring to RE |
|
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.
Thanks!
We should really clean our docstrings to follow the recommendations at https://docs.julialang.org/en/latest/manual/documentation/. For now I'd say it's OK to be consistent with our current style.
@ajwheeler - after this is merged I can do what @nalimilan proposes above, but maybe you would be interested in doing this? |
Sorry, I no longer have time to work on this. Feel free to discard what
I've done if it's easier.
…On Sat, Jun 8, 2019 at 12:34 PM Bogumił Kamiński ***@***.***> wrote:
@ajwheeler <https://github.com/ajwheeler> - after this is merged I can do
what @nalimilan <https://github.com/nalimilan> proposes above, but maybe
you would be interested in doing this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1814?email_source=notifications&email_token=AAFN2GYHQDUHR46C2CWJWZLPZPNQXA5CNFSM4HNZQ5A2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHYCZY#issuecomment-500138343>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFN2GYYUW6EXDGGSKHMUD3PZPNQXANCNFSM4HNZQ5AQ>
.
|
@nalimilan - you are better with docstrings than me 😄 . How do you advise to proceed with this? |
I have opened #2077 instead of this PR. |
closes #1265 unless I'm mistaken