-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
Thanks for the contribution! As an aside, it's best not to use the So that GitHub will auto-close the issue once this is merged: Fixes #159 |
@doc """ | ||
`broadcast!(f, B::NullableArray, As::NullableArray...; lift::Bool=false)` | ||
""" | ||
`broadcast!(f, B::NullableArray, As::NullableArray...; lift::Bool=false)` |
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.
When these are indented, the backticks should be removed.
a04aa1e
to
670daad
Compare
Oh, cool I did not know that about @'s. Thanks! Let me know if there are more changes. |
@doc """ | ||
`nullify!(X::NullableArray, I...)` | ||
""" | ||
nullify!(X::NullableArray, I...) | ||
|
||
This is a convenience method to set the entry of `X` at index `I` to be null | ||
""" -> |
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.
The arrows at the end of the doc strings should also go; they aren't part of the new syntax.
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.
Done, also I noticed I messed up and used 3 space indents so I fixed that as well
471ce16
to
3f8c304
Compare
Current coverage is 84.70% (diff: 100%)@@ master #162 diff @@
==========================================
Files 14 14
Lines 863 863
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 731 731
Misses 132 132
Partials 0 0
|
@davidagold Is the nightly failure a known issue? |
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! Looks good apart from a few details.
@doc """ | ||
`broadcast!(f, B::NullableArray, As::NullableArray...; lift::Bool=false)` | ||
""" | ||
broadcast!(f, B::NullableArray, As::NullableArray...; lift::Bool=false) | ||
This method implements the same behavior as that of `broadcast!` when called on |
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.
Missing line break here.
Base.endof(X::NullableArray) = endof(X.values) # -> Int | ||
|
||
@doc """ | ||
|
||
""" -> |
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 empty comment is weird, probably time to remove it.
@doc """ | ||
`NullableArray{T, N}` is an efficient alternative to `Array{Nullable{T}, N}`. | ||
""" | ||
NullableArray{T, N} is an efficient alternative to `Array{Nullable{T}, N}`. |
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 line isn't a signature, it's just a sentence, so keep it as it was previously.
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 for the thorough review, @nalimilan, and for catching some stuff I missed!
@ararslan The nightly failure is known, that's a problem with |
Woops, completely forgot about this. It's no longer October, but let's pretend it is... |
Shamelessly took the low hanging fruit :)