Skip to content

Conversation

@xorJane
Copy link
Contributor

@xorJane xorJane commented Oct 14, 2016

I added some doctests to sort.jl in issorted(), sort!(), sort(), sortrows(), sortcols(), sortperm(), and sortperm!(). Per make -C doc doctest, it seems all added tests pass.

Many of the examples are redundant. Just let me know if you'd like more/different examples, less redundancy, etc.!

Thanks a lot!!

ref #18821

@mauro3
Copy link
Contributor

mauro3 commented Oct 14, 2016

I think they should be more concise: when calling help at the REPL I don't want pages and pages of examples. For instance there is no advantage to sort vectors with more than three elements. Also construction of the vector should be done inside the function call. For example the issorted example could be shortened to:

```jldoctest
julia> issorted([1,2,3])
true

julia> issorted(-1* [1,2,3])
false

Thanks for your efforts!

@kshyatt kshyatt added the docs This change adds or pertains to documentation label Oct 14, 2016
@StefanKarpinski
Copy link
Member

It's much better to have too much documentation than too little, but I do think that some of the extra examples would be better off in the manual, but then again the stdlib reference is generated from the doc strings. Maybe we need some mechanism for basic and extended documentation?

@xorJane
Copy link
Contributor Author

xorJane commented Oct 15, 2016

Thank you --- I appreciate the feedback!

I updated my PR to make the examples more concise as @mauro3 suggested, and I added doctests for sortperm() and sortperm!()

For now I left vector construction outside of the function calls to sort!() and sortperm!() to illustrate mutation, but I'm happy to change that too. :)

@hayd
Copy link
Member

hayd commented Mar 22, 2017

Another thing worth mentioning is that [-1, -2, -3] is "sorted" in the sense that it is reversed, so it may be worth adding even more examples:

julia> issorted([1, 2, 3])
true

julia> issorted([2, 1, 3])
false

julia> issorted([-1, -2, -3])
false

julia> issorted([-1, -2, -3], rev=true)
true

This branch needs a rebase!

@xorJane
Copy link
Contributor Author

xorJane commented Mar 27, 2017

Thanks for the bump! Just rebased. It looks like most of the functions for which I originally wrote doctests have since been covered. Now this PR just adds doctests for sortrows and sortcols, and adds a doctest using the rev flag to the docstring for issorted as suggested by hayd. Have a great night!

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for updating

@tkelman tkelman merged commit caff424 into JuliaLang:master Mar 27, 2017
@xorJane xorJane deleted the sortdocs branch March 27, 2017 17:49
@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling 78cc622 on xorJane:sortdocs into 55f4417 on JuliaLang:master.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants