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

Get rid of vector of test objects #22

Closed
gdalle opened this issue Oct 2, 2023 · 14 comments
Closed

Get rid of vector of test objects #22

gdalle opened this issue Oct 2, 2023 · 14 comments

Comments

@gdalle
Copy link
Collaborator

gdalle commented Oct 2, 2023

I feel like a single test object would make the code cleaner, eg. by removing the need for a wrapper.
The user can always make the for loop themselves

@rafaqz
Copy link
Owner

rafaqz commented Oct 2, 2023

By wrapper you mean [ ] ?

You're right, currently it's kind of annoying.

The reason it is like this is because the vector was an input to the@implements macro. That made the objects available in a function so we could test at compile time and later build combinatorial interface tests. We sadly dont have that anymore.

Progressively this packake is getting less automated and concise, and less powerful, and Im wondering if that was a mistake.

We should probably at some stage go back having that vector of objects attached to the type via a function, then we could use it programmatically to test other interfaces.

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 2, 2023

But I thought you agreed to that change because it allowed the interface user to define that vector of objects, instead of the interface writer? That's really important in my view

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 2, 2023

And btw, allowing multiple arguments in an interface is quite a power boost, so it's not all doom and gloom

@rafaqz
Copy link
Owner

rafaqz commented Oct 2, 2023

But I thought you agreed to that change because it allowed the interface user to define that vector of objects, instead of the interface writer? That's really important in my view

The person who writes @implements (lets call them the interface implementer) always provided the objects. The interface writer defines @interface - which never had objects.

I allowed the change because the objects are only needed in the tests now and not actually used in @implements, and because over time I had forgotten the reasons I had for them originally, the potential to use them for large scale testing of interface ecosystems. Which I think would be quite powerful.

And btw, allowing multiple arguments in an interface is quite a power boost, so it's not all doom and gloom

Absolutely! Your PRs have been great, Arguments is a solid improvement.

But we do have less information available at compile time for types that implement the interface, and less concrete proof that the interface is tested - than we originally had when I first wrote it.

At some point we will need to have a little discipline to hang on to or add things that on the surface seem annoying but actually have broader potential in the long term.

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 2, 2023

I agree that it does seem a bit silly to be able to declare the interface implemented if no tests pass. So in theory, all it takes is to move Interfaces.test back inside Interface.@implements?

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 2, 2023

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 2, 2023

This issue was initially about dropping the TestObjectWrapper shenanigans. But I guess if the test objects go back to @implements the option to provide several at once is important, because there can only be one macro call

@rafaqz
Copy link
Owner

rafaqz commented Oct 2, 2023

Yeah, it requires reverting half the changes that have been made 😅

@implements needs objects to test, and to output a function that runs tests during precompilation.

There is a point to be made about not having the interface happen in precompile, and Im sure enforcing it will annoy some people.

Maybe we can add a keyword to the macro to not run tests, like @implements SomeInterface :untested, that would make the status of the implementation clear to everyone that reads it, and be available at compile time to other packages in e.g. istested(Interface, obj)

@rafaqz
Copy link
Owner

rafaqz commented Oct 2, 2023

Oops was writing when you posted.

Yeah I think TestObjectWrapper was required for dispatch back when it was used in @implements.

@rafaqz
Copy link
Owner

rafaqz commented Oct 14, 2023

Either way, I'm thinking youre right, it would be good to remove the requirement to have a vector, but to also allow it

@gdalle
Copy link
Collaborator Author

gdalle commented Oct 15, 2023

Not sure how to achieve that without the ugly wrapper

@rafaqz
Copy link
Owner

rafaqz commented Oct 15, 2023

At least its a hidden ugly wrapper...

@rafaqz
Copy link
Owner

rafaqz commented Jul 14, 2024

I'm back to thinking that the vector makes things easier, after implementing test_objects(<:Type{<:Interface}).

Its just easier if its always the same access patter - every type has a vector of test objects.

test_objects(MyInterface) then always returns a Dict with pairs that are all Type => Vector

@gdalle
Copy link
Collaborator Author

gdalle commented Jul 22, 2024

Sounds good!

@rafaqz rafaqz closed this as completed Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants