typelevel / algebra

Experimental project to lay out basic algebra type classes
https://typelevel.org/algebra/
Other
378 stars 69 forks source link

add implicit ADT for skipping serialization tests #129

Closed sritchie closed 8 years ago

sritchie commented 8 years ago

First stab at #123.

sritchie commented 8 years ago

@johnynek changed the name and added the type parameter. Do we still want to thread implicit params through all of the test methods if we're only supporting an actual definition of IsSerializable[T] inside the companion object? The only reason to do that is if we want to allow scoping inside the tests. Might be good to just leave it alone.

ceedubs commented 8 years ago

I'm not sure I quite understand https://github.com/non/algebra/pull/129#issuecomment-163461243. I wouldn't think that this approach would require the instance to be defined in the companion object. Would it? In most cases I don't think we'll have the instance in the companion object because 1) Future is the instance where we've wanted this and 2) tests are probably more likely to rely on the laws module than the main code where classes are defined.

johnynek commented 8 years ago

I think we have to thread the implicit all the way through to take this approach (even if there is an IsSerializable instance for the type) due to type erasure.

I haven't really looked at the hierarchy to see if that will be a disaster.

I agree now with @ceedubs even though I suggested the name. The problem is this is purely type-level function. We aren't going to do anything with it except skip a test at runtime. One way out of the naming thing would be to use Priority. We could do implicit p: Priority[DummyImplicit, NotSerializable[M]] and then only define NotSerializable[Future[T]] for instance. But really, that's just another way to express the ADT in a possibly more indirect way.

I would suggest, to see if this approach works, we set aside the naming question and see how clean it is to plumb it all the way through and have a test for a non-serializable class (maybe something that explicitly throws if it is serialized).

ceedubs commented 8 years ago

@johnynek I think that's a good suggestion. And I now understand why you were saying it would need to be threaded all the way through. I had forgotten that other tests call the serializability test, but that's of course why this is needed in the first place :)

sritchie commented 8 years ago

@johnynek, @ceedubs - take a look and see if you guys can stomach the required changes in LawTests. Before we were explicitly providing implicit parameters. Now that there are two or three per method, I expanded functions out so I could use a locally defined implicit and let the default resolution handle the rest.

Also - it looks like to reuse these tests, users are going to extend LawTestsBase. Currently that means they inherit all of the algebra tests, in addition to the helpers. Do we want to move the actual tests out into their own trait, separate from laws etc?

johnynek commented 8 years ago

I really appreciate you running this idea down, @sritchie . @ceedubs What do you think? do you have any other ideas? I guess a default parameter that is threaded through is the other idea. Is there any fancy scalacheck trick that we could change an argument within a scope at the scalacheck level so we don't have to litter the code with threading all the way through (that may be ugly in its own way).

sritchie commented 8 years ago

Annoying that we can't just add a method at the bottom of the trait heirarchy (inside GroupLaws) that supplies the default implicit, and remove it from the IsSerializable companion object.

If we do that, will any trait that extends the base and overrides that method also pass its new default implicit on to calls to serializable?

Then users can just make a new class with an overridden def for types that can't support serialization. Happy to try it. (My mental ability to anticipate implicit scoping's still rusty :)

sritchie commented 8 years ago

Ah, I guess not, since LawTestsBase doesn't extend that stack.

sritchie commented 8 years ago

Let's close this one up for now. The changes required for this approach are insane. Let me know what you think of the simpler approach here:

https://github.com/non/algebra/pull/141