JuliaGraphs / GraphsBase.jl

Basic interface and structures for the JuliaGraphs ecosystem
http://juliagraphs.org/GraphsBase.jl/
MIT License
11 stars 1 forks source link

Best formalism to define the interface #2

Open gdalle opened 1 year ago

gdalle commented 1 year ago

When we define the new API, there will be abstract types with some methods that must be implemented. In base Julia, I see two main ways of specifying this (as in, defining the function name, possibly with a generic docstring):

julia> function myfunc1 end
myfunc1 (generic function with 0 methods)

julia> myfunc2(args...) = error("Not implemented")
myfunc2 (generic function with 1 method)

julia> myfunc1(1)
ERROR: MethodError: no method matching myfunc1(::Int64)
Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

julia> myfunc2(1)
ERROR: Not implemented
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] myfunc2(args::Int64)
   @ Main ./REPL[2]:1
 [3] top-level scope
   @ REPL[5]:1

I believe a methodless function is better than a single method throwing an error by default. The end result is nearly the same in terms of user experience, but I find the first error clearer because it directly tells you what to implement. @simonschoelly what do you think?

Of course we could also consider more advanced options like Interfaces.jl or InterfaceSpecs.jl but maybe it's a bit overkill?

See also: https://github.com/JuliaGraphs/Graphs.jl/pull/262

henrik-wolf commented 10 months ago

I have been looking at RequiredInterfaces.jl which seems like an option as well. Given that the Graphs interface is quite complex, I am in favour of using some interface package to check for correct implementations. RequiredInterfaces.jl also adds a NotImplementedError which could be used instead of the simple Error: not implemented.

gdalle commented 10 months ago

Yeah since I opened this issue I experimented with RequiredInterfaces and was very satisfied. I would love to see it in action here, plus the customer support I got from the lead dev was top notch. Once the first PR is merged I'll try to add it

gdalle commented 9 months ago

I'm also curious as to whether traits are actually needed or not. Some are used for dispatch, like is_directed, but some others just forbid operations that could also be prevented with an explicit error.

gdalle commented 9 months ago

Regarding drawbacks of traits: