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

[bugfix] Base.eltype works on graph types as well as instances #144

Merged
merged 10 commits into from
Jun 16, 2023

Conversation

lmondada
Copy link
Contributor

According to the Julia documentation, eltype should be defined on the ::Type{AbstractGraph} instead of the instances themselves:

"The definition eltype(x) = eltype(typeof(x)) is provided for convenience so that instances can be passed instead of types. However the form that accepts a type argument should be defined for new types."

Currently, however:

julia> eltype(SimpleGraph{Int64})
Any

julia> eltype(SimpleGraph{Int64}())
Int64

This PR fixes this to return

julia> eltype(SimpleGraph{Int64})
Int64

Copy link
Member

@etiennedeg etiennedeg left a comment

Choose a reason for hiding this comment

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

It makes me wonder if it's okay to call this method eltype since AbstractGraph is not iterable. I don't think it is too bad, we should better not break anything.

test/simplegraphs/simplegraphs.jl Outdated Show resolved Hide resolved
@gdalle
Copy link
Member

gdalle commented May 1, 2023

I removed the implementations for Simple(Di)Graph and moved the fallback to AbstractGraph. As a result, eltype does not need to be part of the interface anymore

@gdalle gdalle requested a review from etiennedeg May 1, 2023 06:30
@gdalle gdalle removed the request for review from etiennedeg May 1, 2023 08:05
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Merging #144 (90afd95) into master (5649092) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   97.23%   97.26%   +0.02%     
==========================================
  Files         114      114              
  Lines        6583     6579       -4     
==========================================
- Hits         6401     6399       -2     
+ Misses        182      180       -2     

@gdalle gdalle added the bug Something isn't working label Jun 16, 2023
@gdalle gdalle merged commit 11f54ad into JuliaGraphs:master Jun 16, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants