Moved test objects to test method

[stemann]

As discussed in #9

[codecov-commenter]

Codecov Report

Merging #10 (8dc5408) into main (15cd55f) will decrease coverage by 4.13%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   81.48%   77.35%   -4.13%     
==========================================
  Files           3        4       +1     
  Lines         108      106       -2     
==========================================
- Hits           88       82       -6     
- Misses         20       24       +4     

Impacted Files	Coverage Δ
src/test.jl	77.77% <62.50%> (-6.64%)	:arrow_down:
src/implements.jl	81.25% <100.00%> (+7.33%)	:arrow_up:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rafaqz]

Thanks. It definitely feels slicker to move those definitions into the test function and keep the declaration lean.

I'm just wondering if we lose anything?

I think my original idea was that a user could run test(TheInterface, TheObjectType) without needing to construct test objects. But probable that's an extremely rare situation in practice.

If you cant think of any other downside to this, lets merge it.

[stemann]

Thanks. It definitely feels slicker to move those definitions into the test function and keep the declaration lean.

I'm just wondering if we lose anything?

I think my original idea was that a user could run test(TheInterface, TheObjectType) without needing to construct test objects. But probable that's an extremely rare situation in practice.

If you cant think of any other downside to this, lets merge it.

I think it is better to require that test objects are provided/created with the tests.

It could be encouraged that packages somehow make test values/objects available for themselves and for downstream packages if that was/is your concern? I.e. was your concern with the ease of interface testing wrt. sub-types of a hierarchy where an interface is defined for a supertype?

[stemann]

I think you were right saying (in #9) that the interface definitions should work on types - not values/objects. There's still a few mentions of objects - that could probably be cleaned up in a follow-up.

[stemann]

Also, JET.report_package(; target_defined_modules = true) is not completely happy with the calls to components(T) - not sure why...

[rafaqz]

Thanks for checking JET. Can you post the output?

Probably my concern over test objects isn't that important for now, and we should follow YAGNI.

[stemann]

Thanks for checking JET. Can you post the output?

Probably my concern over test objects isn't that important for now, and we should follow YAGNI.

Probably not related to this branch:

═════ 4 possible errors found ═════ ┌ @ /Users/jsa/.julia/dev/Interfaces/src/interface.jl:20 Interfaces.components(T) │ no matching method found components(::Type{<:Interfaces.Interface}): Interfaces.components(T::Type{<:Interfaces.Interface}) └──────────────────────────────────────────────────────── ┌ @ /Users/jsa/.julia/dev/Interfaces/src/interface.jl:22 Interfaces.components(T) │ no matching method found components(::Type{<:Interfaces.Interface}): Interfaces.components(T::Type{<:Interfaces.Interface}) └──────────────────────────────────────────────────────── ┌ @ /Users/jsa/.julia/dev/Interfaces/src/test.jl:39 Interfaces.components(T) │ no matching method found components(::Type{<:Interfaces.Interface}): Interfaces.components(T::Type{<:Interfaces.Interface}) └─────────────────────────────────────────────────── ┌ @ /Users/jsa/.julia/dev/Interfaces/src/test.jl:48 Interfaces.components(T) │ no matching method found components(::Type{<:Interfaces.Interface}): Interfaces.components(T::Type{<:Interfaces.Interface}) └───────────────────────────────────────────────────

[rafaqz]

Oh right JET wants us to define a method for the Interface supertype. I guess that could just return false to make it happy.

[stemann]

Yet another consideration following this change: I think I would prefer if the arguments for @implements and implements were ordered in reverse: @implements(type, interface_type) - as in https://github.com/quinnj/Interfaces.jl/blob/main/test/runtests.jl#L38

[rafaqz]

Haha I guess that's a matter of taste, maybe I think about it the other way around.

If you think its better, I'll merge the PR, clearly Jacob thinks its better too.

[rafaqz]

Also I find it hilarious how similar @quinnj s implementation is - I didn't read it before writing this. Some differences in design, but all the same function, types and macros, how is that possible

[stemann]

He he - that's funny indeed :-)

But isn't (at least) the interface definition a bit different in only declaring method signatures? Where the predicate approach can check much more complex interactions?

@interface Iterable begin

    iterate(::Iterable)
    iterate(::Iterable, st)

    Base.IteratorSize(::Type{Iterable})::Union{Base.HasLength, Base.HasShape, Base.IsInfinite, Base.SizeUnknown}

    if Base.IteratorSize(Iterable) == Base.HasLength()
        length(::Iterable)
    elseif Base.IteratorSize(Iterable) isa Base.HasShape
        length(::Iterable)
        size(::Iterable)
    end

    Base.IteratorEltype(::Type{Iterable})::Union{Base.HasEltype, Base.EltypeUnknown}

    if Base.IteratorEltype(Iterable) == Base.HasEltype()
        eltype(::Iterable) || eltype(::Type{Iterable})
    end

end

[rafaqz]

I really havent read it as closely as you, I meant on a really surface level scan of the code.

That syntax does look nice though... but do you mean it cant actually check return values?

I thought the main difference overall was that my version gives other packages a breakdown of the implementation of each interface as traits.

(My idea was really a hacked together Haskell typeclasses thing where it was knowable at compile time what objects had what behaviours. The other parts of the design are a bit more random.)

[rafaqz]

@stemann (now I remember, lol) having Duck() in the @interface macro was legacy from when I used to run test automatically in precompilation.

I really liked the idea of having it all automated so the interface was more reliable. But people didn't like the potential overheads of that approach, so it was dropped and now you have to run test manually.

[stemann]

@stemann (now I remember, lol) having Duck() in the @interface macro was legacy from when I used to run test automatically in precompilation.

I really liked the idea of having it all automated so the interface was more reliable. But people didn't like the potential overheads of that approach, so it was dropped and now you have to run test manually.

Ah, yes - so this PR actually supports the change you made in #8.

While digging around, I also noticed your reasoning in https://github.com/lorenzoh/Invariants.jl/issues/2#issuecomment-1143648359

[stemann]

I really havent read it as closely as you, I meant on a really surface level scan of the code.

That syntax does look nice though... but do you mean it cant actually check return values?

I thought the main difference overall was that my version gives other packages a breakdown of the implementation of each interface as traits.

(My idea was really a hacked together Haskell typeclasses thing where it was knowable at compile time what objects had what behaviours. The other parts of the design are a bit more random.)

Yeah, I think that that syntax also merits some consideration.

It's been a while since I looked at https://github.com/quinnj/Interfaces.jl - but I would think that it can check what is specified in the interface - that methods exist and with the proper signatures - including return value types, but not return values, no. Probably worth getting back to in #9 (or a follow-up - to get #9 closed).

In my view, your version provides a much more flexible interface specification - covering all of:

1. An empty interface specification, e.g. just defining that AnimalInterface is implemented wrt. Ducks - with no mandatory predicates.
2. A signature interface specification, where mandatory predicates check that methods exist and return the proper types.
3. A detailed interface specification, where mandatory predicates check that methods not only exist and return the proper types, but also check various interactions among methods, e.g. to support defining interface behaviour instead of explaining it as in https://github.com/IHPSystems/Cameras.jl/issues/7#issuecomment-1546876405
4. An interface specification with optional parts

... though I'm still not completely clear if interfaces with optional parts is better than separating optional parts into separate interface specifications (cf. #9).

[rafaqz]

I think the last point will need to be resolved by implementing a bunch of interfaces for base and package types.

There are often kind of irrelevent parts of an interface that are nevertheless still part of it. For example a DimensionalData.jl AbstractDimArray can keep dims to track what slice an array came from, or disguard them. Theyre pretty much irrelevant, but its nice to see in plot labels. And if you do implement it, you want the plot labels to be right.

It felt really clunky to me define an interface spec for these tiny things separately. My hunch is that people wont do it unless it also feels trivial.

[quinnj]

Note there is also https://github.com/Keno/InterfaceSpecs.jl with similar ideas

[rafaqz]

Also with what looks like proof solver built in 😂

Nice to have that "ok there are smarter people working on this now" feeling

[stemann]

Also with what looks like proof solver built in 😂

Nice to have that "ok there are smarter people working on this now" feeling

Indeed! 😃 I think I’ve finally grasped what Keno is saying - after the third read or so :-)

But it might be worthy of a separate issue to consider the suggestion of being open to extension with proof engines.

… and the forall 👀

IIUC Keno shared his thoughts to “aid the direction” of, e.g. these efforts.