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

Remove a convert method #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove a convert method #52

wants to merge 3 commits into from

Conversation

davidanthoff
Copy link
Member

Fixes #51.

@JeffBezanson I guess this would be the PR, but it fails tests with the method ambiguity error. Opening a PR so that we can see what happens on julia nightly.

@JeffBezanson
Copy link

Ok, it looks like a recent change to type intersection changed this, but I'm not totally confident it won't change again, so let's just keep the method for now.

@codecov-io
Copy link

Codecov Report

Merging #52 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #52      +/-   ##
=========================================
- Coverage   74.95%   74.9%   -0.05%     
=========================================
  Files          13      13              
  Lines         519     518       -1     
=========================================
- Hits          389     388       -1     
  Misses        130     130
Impacted Files Coverage Δ
src/scalar/core.jl 92.59% <ø> (-0.06%) ⬇️

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 7f32c4d...6c55dfb. Read the comment docs.

@davidanthoff
Copy link
Member Author

I think removing the method seems to work on both julia 1.1 and 1.2, so maybe that is the right way to do it for now?

@tkf
Copy link

tkf commented Oct 8, 2019

Would it be crazy to dynamically determine if you need disambiguation at precompile-time? Something like this:

try
    convert(Any, NA)
    true
catch err
    err isa MethodError || rethrow()
    false
end || begin
    Base.convert(::Type{Any}, ::DataValue{Union{}}) = NA
end

@davidanthoff
Copy link
Member Author

That seems very involved :) I almost feel having the static version check there is simpler?

@tkf
Copy link

tkf commented Oct 9, 2019

I thought it's nice to be able to define it only using stable public API; i.e., it does/will not have to record which past/future Julia versions need this definition.

@davidanthoff
Copy link
Member Author

@JeffBezanson, do you have an opinion about that kind of approach?

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.

remove convert method for Any?
4 participants