Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Apr 24, 2021

Now Origin accepts Integer arguments that may be coerced to Int.

After this PR:

julia> Origin(Int32(1))
Origin{Int64}(1)

Also disallow stuff like Origin(("abc",))

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #229 (35833ca) into master (b74ae55) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   94.40%   94.42%   +0.01%     
==========================================
  Files           5        5              
  Lines         322      323       +1     
==========================================
+ Hits          304      305       +1     
  Misses         18       18              
Impacted Files Coverage Δ
src/origin.jl 100.00% <100.00%> (ø)

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 b74ae55...35833ca. Read the comment docs.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Maybe we can add a test_throw on Float64 value. (Ignore this if we already have one)

@jishnub
Copy link
Member Author

jishnub commented Apr 24, 2021

Should we just convert all numbers to Ints here? Why restrict?

@johnnychen94
Copy link
Member

Should we just convert all numbers to Ints here? Why restrict?

I don't see any drawbacks of this since it will throw meaningful enough errors if the conversion fails.

@jishnub jishnub merged commit 8b4cc47 into JuliaArrays:master Apr 24, 2021
@jishnub jishnub deleted the originInteger branch April 24, 2021 16:58
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.

2 participants