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

Restrict zero function types #44

Merged
merged 1 commit into from
Oct 12, 2017
Merged

Restrict zero function types #44

merged 1 commit into from
Oct 12, 2017

Conversation

cjprybol
Copy link
Contributor

Base only provides zero(::Type{T}) where T<:Number, not zero(::Type{T}) where T, and our method results in StackOverflows when called with Any when it should return an error. See JuliaData/DataFrames.jl#1251

Base only provides `zero(::Type{T}) where T<:Number`, not `zero(::Type{T}) where T`, and our method results in StackOverflows when called with `Any` when it should return an error. See JuliaData/DataFrames.jl#1251
@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #44   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           1        1           
  Lines          89       89           
=======================================
  Hits           82       82           
  Misses          7        7
Impacted Files Coverage Δ
src/Nulls.jl 92.13% <100%> (ø) ⬆️

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 f54c091...4111551. Read the comment docs.

@quinnj quinnj merged commit 9756fed into master Oct 12, 2017
@quinnj quinnj deleted the cjprybol-patch-1 branch October 12, 2017 02:56
@nalimilan
Copy link
Member

@quinnj Please don't merge PRs during the night! Unless it's blocking something, I think a PR should sit for at least one day in case somebody has remarks.

We had already spotted the problem before introducing zero (https://github.com/JuliaData/Nulls.jl/pull/31/files#r141323811), but merged it anyway because zero doesn't not only work on Number. At the very least, it should also work for date types. Then it's debatable whether it's OK to add more methods for each type which might support zero, or whether we really need a generic fallback despite the StackOverflow error it triggers.

@quinnj
Copy link
Member

quinnj commented Oct 12, 2017

Sorry, I thought it had been open longer. I feel like we should be more conservative here. We can add a method for Union TimeTypes and more as needed, but I feel like it's too easy to bump into zero calls out in the wild to be triggering stack overflows.

@nalimilan
Copy link
Member

We should have a policy for this kind of situation. For example, we currently define * only for Number, so it doesn't work for strings. If we decide to overload functions on a case by case basis, we should add AbstractString methods. The other approach is to accept Any arguments.

@nalimilan
Copy link
Member

See #45.

@nalimilan nalimilan mentioned this pull request Nov 29, 2017
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.

4 participants