sviperll / adt4j

adt4j - Algebraic Data Types for Java
BSD 3-Clause "New" or "Revised" License
143 stars 8 forks source link

Provide support for base interfaces/classes. #18

Closed talios closed 9 years ago

talios commented 9 years ago

This allows for providing abstractions over common disparate ADT values, and offering a simple way of providing extension methods directly on the generated value classes.

I'm not 100% happy with using String's to refer to the baseClass or baseInterface attributes ( you can see the example in the BaseOptionalVisitor.java file, but it's been a long since I've use JCodeModel or written an annotation processor so not sure if it's possible to refer to the .class objects directly here ( and fall into the type mirrors API ) so maybe you have some better ideas/thoughts.

Basically, this gives ADT4j the ability to declare a base/marker interface for the generated value class, which would allow for some common abstractions to be implemented. As part of this change the generic declaration for the exception type is moved down to the accept method, otherwise the base class can't provide an generified instance of of a visitor.

All tests as they are continue to pass.

sviperll commented 9 years ago

Nice trick with abstract accept method :) I'll merge it, but I'll try to convert string arguments to class references before release.

Besides, what is the point of defining toString in base class as it will be overridden anyway?

sviperll commented 9 years ago

I've refactored your implementation somehow and converted strings to class-literals see https://github.com/sviperll/adt4j/commit/e98914acd143a2724514c425b1ed5a9872081dc9.

But still I think that it is much more clear and easy to extend generated class instead of making generated class extend some predefined class. What are your objections against extending generated class?

jbgi commented 9 years ago

I would also very much prefer that generated class extend predefined class. So this is a big +1 for this feature.

sviperll commented 9 years ago

Now we have both :)

jbgi commented 9 years ago

Yep! I'm going to test this feature asap!

jbgi commented 9 years ago

Ok, so this is my wish list for this feature that will maximize its value in my eyes:

please tell me if I can be of any help (I can probably try to implement the first two items).

talios commented 9 years ago

Mmm, @jbgi why would selfReferenceVariableName not be needed anymore? You'd still want to know you're say an Optional and not some OptionalBaseSupportClass.

The main reason here is one of the reasons I was wanting this functionality - if you have several ADT4j based classes in your system that all wrap say <T> one could have a generic Foldable base class/interface, that provides map and flatMap methods etc.

Here, you'd still want to know that your self type is, as THAT's what you want to use in your recursive datatypes ( generally the only reason to ever declare the selfReferenceVariableName in the first place ) - unless I'm missing something ( it's getting late here, starting to not think perfectly straight ).

In the case of using extendsInterface ( and using Java 8 ) one can use default methods without needing to define an abstract accept method ( IMHO extendsClass is a stop gap measure until one can shift to Java 8 ).

@sviperll Regarding the toString() on the BaseOptionalSupport.java file - that can actually be removed, I was following the code changes I was making in the Main.java and moved to toString(optional) method to an embedded one - not thinking that toString() would be overridden ;p