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

More doctests and cleanup for intfuncs #22515

Merged
merged 10 commits into from
Jun 27, 2017
Merged

More doctests and cleanup for intfuncs #22515

merged 10 commits into from
Jun 27, 2017

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jun 24, 2017

No description provided.

@kshyatt kshyatt added domain:docs This change adds or pertains to documentation test This change adds or pertains to unit tests labels Jun 24, 2017
@kshyatt kshyatt requested a review from KristofferC June 24, 2017 21:29

# Examples
```jldoctest
julia> get(d, "a", 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this pass? where does d come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, forgot to copy paste

@tkelman tkelman assigned tkelman and unassigned tkelman Jun 25, 2017
@tkelman tkelman self-requested a review June 25, 2017 01:43
Copy link
Sponsor Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

LGTM after one read through. It overlaps a bit with #22289 but no problems.

@KristofferC KristofferC mentioned this pull request Jun 25, 2017
@KristofferC
Copy link
Sponsor Member

Think this should be valid to backport.

base/array.jl Outdated

```julia-repl
julia> resize!([6, 5, 4, 3, 2, 1], 8)
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't guaranteed to always be zero-initialized beyond the end of the array, is it?

Copy link
Member

Choose a reason for hiding this comment

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

No, I guess thats why this wasn't a doctest.

@@ -1772,11 +1967,12 @@ Return the value stored for the given key, or if no mapping for the key is prese
`key => f()`, and return `f()`.

This is intended to be called using `do` block syntax:

```julia
get!(dict, key) do
Copy link
Contributor

Choose a reason for hiding this comment

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

if fenced with triple backticks, then shouldn't be indented any more

# Examples

```jldoctest
julia> searchsortedlast([1, 2, 4, 5, 4], 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

examples for this function should use sorted inputs, that's what it's designed for

@@ -2124,6 +2338,14 @@ throw
⊊(a,b) -> Bool

Determine whether every element of `a` is also in `b`, using [`in`](@ref).

```jldoctest
Copy link
Contributor

Choose a reason for hiding this comment

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

# Examples


# Example
```jldoctest
julia> Base.SparseArrays.dropstored!(A, [1, 2], [1, 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

where does A come from?

@kshyatt
Copy link
Contributor Author

kshyatt commented Jun 26, 2017

OK to go?

base/array.jl Outdated
1
0
0
julia> a = resize!([6, 5, 4, 3, 2, 1], 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like there's an extra leading space in this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and I ran make check-whitespace locally.

Copy link
Contributor

@tkelman tkelman Jun 26, 2017

Choose a reason for hiding this comment

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

how strange, I see bdbcbba from a fetch, but github isn't showing it in the pr yet? problem on github's side https://status.github.com/

@@ -483,6 +499,23 @@ Returns the range of indices of `a` which compare as equal to `x` (using binary
according to the order specified by the `by`, `lt` and `rev` keywords, assuming that `a`
is already sorted in that order. Returns an empty range located at the insertion point
if `a` does not contain values equal to `x`.

# Examples

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line?

@@ -786,6 +859,48 @@ at the position where it would appear if the array were fully sorted via a non-s
algorithm. If `k` is a single index, that value is returned; if `k` is a range, an array of
values at those indices is returned. Note that `select!` does not fully sort the input
array.

# Examples

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line here as well? (Have we a convention for absence/presence of an extra empty line following # Example[s]? Remarked here and above for consistency with the absence earlier in this PR, but have no preference.)

Copy link
Contributor

Choose a reason for hiding this comment

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

we should pick and standardize - personally I lean towards leaving out the empty line, assuming it should render the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but I think we should a) pick one and b) have one mega-PR to fix it everywhere (much like @StefanKarpinski did with the @test_approx_eq macro).

Copy link
Member

@Sacha0 Sacha0 Jun 26, 2017

Choose a reason for hiding this comment

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

Sounds good! :) (Small PRs are nice though!)

3
2
1
```
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an example illustrating the partial sorting behavior would be worthwhile? The examples above yield fully sorted results.

@@ -795,6 +910,15 @@ select!
Create a random ASCII string of length `len`, consisting of upper- and
lower-case letters and the digits 0-9. The optional `rng` argument
specifies a random number generator, see [Random Numbers](@ref).

# Example

Copy link
Member

Choose a reason for hiding this comment

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

See extra empty line question above :).

@@ -1179,6 +1345,20 @@ delete!

Returns the index of the first value in `a` greater than or equal to `x`, according to the
specified order. Returns `length(a)+1` if `x` is greater than all values in `a`.
`a` is assuemd to be sorted.
Copy link
Member

Choose a reason for hiding this comment

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

assuemd? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops

`a` is assuemd to be sorted.

# Examples

Copy link
Member

Choose a reason for hiding this comment

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

See question above re. empty lines :).

@@ -111,6 +142,18 @@ module IteratorsMD

Consequently these can be useful for writing algorithms that
work in arbitrary dimensions.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an # Example heading?

Copy link
Contributor

Choose a reason for hiding this comment

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

this wasn't addressed?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

The new examples are a wonderful addition! :)

@kshyatt
Copy link
Contributor Author

kshyatt commented Jun 27, 2017

Unless someone requests changes or beats me, I'm going to merge in the morning :)

@KristofferC KristofferC merged commit de74614 into master Jun 27, 2017
@kshyatt kshyatt deleted the ksh/doctestint branch June 27, 2017 03:01
ararslan pushed a commit that referenced this pull request Sep 11, 2017
* More doctests and cleanup for intfuncs

* Also more doctests for arrays

* More doctests and Example{s} inserting

* Doctests for DAYS

* Fix some failing doctests

* "fix" failing OffsetArray test

* Address more comments

* fix dropstored!

* Fix leading space

* fix typo

Ref #22515
(cherry picked from commit de74614)
ararslan pushed a commit that referenced this pull request Sep 13, 2017
* More doctests and cleanup for intfuncs

* Also more doctests for arrays

* More doctests and Example{s} inserting

* Doctests for DAYS

* Fix some failing doctests

* "fix" failing OffsetArray test

* Address more comments

* fix dropstored!

* Fix leading space

* fix typo

(cherry picked from commit de74614)
vtjnash pushed a commit that referenced this pull request Sep 14, 2017
* More doctests and cleanup for intfuncs

* Also more doctests for arrays

* More doctests and Example{s} inserting

* Doctests for DAYS

* Fix some failing doctests

* "fix" failing OffsetArray test

* Address more comments

* fix dropstored!

* Fix leading space

* fix typo

(cherry picked from commit de74614)
ararslan pushed a commit that referenced this pull request Sep 15, 2017
* More doctests and cleanup for intfuncs

* Also more doctests for arrays

* More doctests and Example{s} inserting

* Doctests for DAYS

* Fix some failing doctests

* "fix" failing OffsetArray test

* Address more comments

* fix dropstored!

* Fix leading space

* fix typo

(cherry picked from commit de74614)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants