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

Throw an error when encountering missing values #183

Merged
merged 1 commit into from
Sep 14, 2022
Merged

Conversation

MikeInnes
Copy link
Member

@MikeInnes MikeInnes commented May 12, 2022

JET.jl rightly notes that this function will error if a missing appears inside an expression, and we try to bind it. This is certainly an edge case (and clearly no-one's hit it so far), but strictly speaking missing is allowed in Exprs, and the internal error isn't ideal if someone does hit it. So I've added an explicit error. In future we could add support (probably by assuming missing == missing for the purposes of this check).

Separately: unfortunately, JET/Julia's inference isn't smart enough to figure out that the missing-boolean error no longer applies. Is there some way to assert that env[name]::!Missing, ie anything other than missing? A type assert would be ok, but I'd be a bit reluctant to do anything more complex that sacrifices readability.

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #183 (6972738) into master (639d1a6) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
+ Coverage   74.68%   74.75%   +0.06%     
==========================================
  Files           9        9              
  Lines         403      404       +1     
==========================================
+ Hits          301      302       +1     
  Misses        102      102              
Impacted Files Coverage Δ
src/match/match.jl 94.11% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639d1a6...6972738. Read the comment docs.

Copy link
Collaborator

@cstjean cstjean left a comment

Choose a reason for hiding this comment

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

Should we merge this?

@MikeInnes MikeInnes merged commit ab2c02d into master Sep 14, 2022
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.

None yet

3 participants