rafaqz / Interfaces.jl

Macros to define and implement interfaces, to ensure they are checked and correct.
MIT License
72 stars 4 forks source link

add InterfacesCore subpackage #39

Closed rafaqz closed 5 months ago

rafaqz commented 5 months ago

This PR adds an InterfacesCore.jl subpackage.

All it implements is abstract type Interface end and a @interface_type macro.

The reason for this is so we can define the interface type without any package overheads, and add Interfaces.jl as an extension. This means packages can just go using Interfaces and test their implementation, rather than needing to load another package.

We cant use extensions currently because the type would be defined in the extension, and not be accessible to anyone.

gdalle commented 5 months ago

Not sure I understand the rationale here, can you elaborate?

rafaqz commented 5 months ago

Ok. So I want to add Interfaces.jl to a package, without adding a full dependency on it or adding any overheads.

Theyre not so bad, but I imagine people will want a no cost option initially. And if all the interface tests get really long it will have some small overhead.

So the options to do that are:

  1. make a separate interfaces package
  2. use an extension

Its kind of annoying to have to do 1 everywhere. But 2 doesn't actually work currently, because if you use the @interface macro in an extension, the types it generates are hidden in the extension and not available in the package.

So we need to define the interface type in the main package, e.g. in DimensionalData.jl.

So:

  1. We move the Interface type that all interfaces depend on to InterfacesCore.jl, with a small macro for adding core interfaces.
  2. Depend on InterfacesCore.jl in DimensionalData.jl as a regular dependency (its tiny).
  3. Use the slimmed down @interface_core macro to define our interface type DimArrayInterface without any tests inside DimensionalData.jl scope
  4. We define an extension DimensionalDataInterfacesExt.jl in DimensionalData.jl
  5. The extension loads when the full Interfaces.jl package is loaded
  6. We import the DimArrayInterface type into the extension from DimensionalData.jl
  7. We define the full interface with @interface ... in the extension using the existing interface type
  8. Packages that want to test the interface have to load Interfaces.jl to get the test function, so they will always have the full DimensionalData.jl interfaces available.

I guess this should be the package readme

gdalle commented 5 months ago

Is an 8-step procedure really worth the trouble of what is probably a very small package overhead? If Interfaces.jl is to be widely adopted it must be very simple, and that split makes it so much more complicated.

gdalle commented 5 months ago

Can we quantify the overhead somehow?

rafaqz commented 5 months ago

Yeah, maybe not worth it its very little overhead. This is Interfaces.jl + DimensionalDataInterfacesExt.jl

julia> using DimensionalData

julia> @time using Interfaces
  0.004137 seconds (4.39 k allocations: 334.969 KiB)

But didn't you put the Graphs.jl interface in a separate package? That's the problem I'm trying to solve.

gdalle commented 5 months ago

But didn't you put the Graphs.jl interface in a separate package? That's the problem I'm trying to solve.

Two reasons for that

But I didn't have to put the checks in the same place as the definition. Ideally the definition should go in Graphs.jl and the checks in each implementing package, as God intended

rafaqz commented 5 months ago

It just feels a bit weird putting the interface tests in runtime code with the rest of the package, and I'm guessing some people wont like that. But maybe its not an issue

gdalle commented 5 months ago

I think we'll cross that bridge if we get to it. What I like about your approach is that the design is dead simple, let's not overcomplicate it to gain 4 ms.

If you really care about moving the tests out of runtime code, maybe there could be an extension of Interfaces.jl that depends on Test being loaded?

rafaqz commented 5 months ago

Yes could be interesting to use Test as the weak dep. Have to think about how that works.

For now I'll move the DimensionalData.jl interface to the package.

rafaqz commented 5 months ago

And your right keeping it dead simple might be the most important thing here.

Thanks for the feedback