added default option to get_prop

[henrik-wolf]

Had to use the has_prop(g, e, :prop) ? get_prop(g, e, :prop) : default_value pattern way too much, found issue #37, and decided to fix this problem. Here is my very simple solution.

Given that the default value has a type, we could maybe even do something like:
get_prop(g, e, prop, default::T)::T where T = has_prop(....) ? get_prop(....) : default
making it typestable, which could be nice.
On the other hand, this would exclude all the cases, where properties can have wildly different types from using this feature. Given that MetaGraphsNext.jl is the typestable successor to this package, it might be nice to not constrain the users in that way.

Not sure though. What do you think?

[gdalle]

I think it's a good idea, and it doesn't hurt users since it's just an additional method signature

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c5aa8e2) 98.69% compared to head (b15fad5) 98.71%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   98.69%   98.71%   +0.02%     
==========================================
  Files           5        5              
  Lines         382      388       +6     
==========================================
+ Hits          377      383       +6     
  Misses          5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[henrik-wolf]

I added a check for existence of a vertex or edge and throw an ArgumentError if the vertex or edge does not exist, with an appropriate error message. I think this is great, as it tells you immediately what you did wrong, but is somewhat inconsistent with the error you get for the "non-default" get_prop method:

julia> get_prop(mg, 2, 4, :testedgeprop)
ERROR: KeyError: key :testedgeprop not found
Stacktrace:
 [1] getindex(h::Dict{Symbol, Any}, key::Symbol)
   @ Base ./dict.jl:484
 [2] get_prop
   @ ~/.julia/dev/MetaGraphs/src/MetaGraphs.jl:277 [inlined]
 [3] get_prop(g::MetaGraph{Int64, Float64}, u::Int64, v::Int64, prop::Symbol)
   @ MetaGraphs ~/.julia/dev/MetaGraphs/src/MetaGraphs.jl:292
 [4] top-level scope
   @ REPL[50]:1

julia> get_prop(mg, 2, 4, :testedgeprop, "placeholder")
ERROR: ArgumentError: Edge 2 => 4 not in graph
Stacktrace:
 [1] get_prop(g::MetaGraph{Int64, Float64}, e::Graphs.SimpleGraphs.SimpleEdge{Int64}, prop::Symbol, default::String)
   @ MetaGraphs ~/.julia/dev/MetaGraphs/src/MetaGraphs.jl:288
 [2] get_prop(g::MetaGraph{Int64, Float64}, u::Int64, v::Int64, prop::Symbol, default::String)
   @ MetaGraphs ~/.julia/dev/MetaGraphs/src/MetaGraphs.jl:293
 [3] top-level scope
   @ REPL[51]:1

which throws a KeyError, since props(g, nonexistent_edge) always returns an empty Dict.
For consistency, props and get_prop should always error with the same error message if accessed at non-existent vertices or edges. I could add the check to get_prop without default argument, but I am not sure how that might impact performance. (Is changing the type of an exception a breaking change?) adding something like that to props would definitely be breaking.

[gdalle]

I think we could just use the get method with a default argument on the dictionary returned by props? That would make for shorter and more coherent code

[henrik-wolf]

you mean something like this

get_prop(graph, edge, prop, default) = get(props(graph, edge), prop, default)

?
That would still result in default being returned even if edge does not exist.

[gdalle]

Yeah but at least like you pointed out it's consistend with the rest of the implem