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

Add isnothing #29679

Merged
merged 6 commits into from
Oct 18, 2018
Merged

Add isnothing #29679

merged 6 commits into from
Oct 18, 2018

Conversation

kescobo
Copy link
Contributor

@kescobo kescobo commented Oct 16, 2018

This adds an isnothing() function to Base to mirror ismissing() and isnan() (see #29674)

I thought it would be pretty straightforward, but... I was naive.

First, I wasn't sure where to put it. Near notnothing() seemed like a good candidate, but I noticed that notnothing() is not exported. ismissing() is defined here near the definition of Missing

struct Missing end

"""
    missing
The singleton instance of type [`Missing`](@ref) representing a missing value.
"""
const missing = Missing()

"""
    ismissing(x)
Indicate whether `x` is [`missing`](@ref).
"""
ismissing(::Any) = false
ismissing(::Missing) = true

But Nothing seems to be defined in boot.jl, which doesn't seem like the best place for this function. Any advice on location from someone that knows the code better than me (eg. anyone at all) would be appreciated. And if it's not in boot.jl, where should I export it from? (I can't find where ismissing() is exported for example).

Second, folks on slack said that doing x === nothing was the most performant way to do this check, so I defined it as

isnothing(x::Any) = x === nothing ? true : false

But the way ismissing is defined above seems way more elegant. Should it be:

isnothing(::Any) = false
isnothing(::Nothing) = true

instead?

Finally, I'm having trouble building julia locally, so I can't do all the steps in CONTRIBUTING.md. I can't imagine how this could cause something to break, but I've been wrong with simple things before. Apologies if I'm missing something important.

@JeffBezanson
Copy link
Member

x === nothing ? true : false is equivalent to just x === nothing, since === always returns a Bool. That said, I think the 2-method implementation ala ismissing is a bit better.

@kescobo
Copy link
Contributor Author

kescobo commented Oct 16, 2018

Oh man, obviously 🤦‍♂️ . I feel like an idiot

test/some.jl Outdated

# isnothing()

using Base: isnothing
Copy link
Member

Choose a reason for hiding this comment

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

I'd add this to base/exports.jl next to ismissing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ararslan check.

@@ -40,6 +40,15 @@ Throw an error if `x === nothing`, and return `x` if not.
notnothing(x::Any) = x
notnothing(::Nothing) = throw(ArgumentError("nothing passed to notnothing"))

"""
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be added to the documentation so it shows up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - I added it next to Nothing.

@KristofferC KristofferC added the needs news A NEWS entry is required for this change label Oct 17, 2018
@kescobo
Copy link
Contributor Author

kescobo commented Oct 17, 2018

@KristofferC Should I be the one to add to NEWS.md?

@KristofferC
Copy link
Member

It would be good :) But if not, the label is here to make sure we do it before the release.

@kescobo
Copy link
Contributor Author

kescobo commented Oct 17, 2018

Check.

@kescobo
Copy link
Contributor Author

kescobo commented Oct 17, 2018

One last (hopefully) question: should I be the one to squash (to comply with point 2 here) or will that be done when merging?

@fredrikekre
Copy link
Member

Can be done on merge.

@JeffBezanson
Copy link
Member

Neither of the items under "New language features" currently in NEWS are new language features, but that can be edited separately.

@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Oct 17, 2018
@kescobo
Copy link
Contributor Author

kescobo commented Oct 17, 2018

Heh - I didn't think so either, but the NEWS.md is pretty sparse, and that seemed more appropriate than any of the other headers ¯_(ツ)_/¯

@JeffBezanson JeffBezanson merged commit d401033 into JuliaLang:master Oct 18, 2018
fredrikekre added a commit that referenced this pull request Nov 30, 2018
fredrikekre added a commit that referenced this pull request Nov 30, 2018
fredrikekre added a commit that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 3, 2018
fredrikekre added a commit that referenced this pull request Dec 4, 2018
fredrikekre added a commit that referenced this pull request Dec 4, 2018
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants