smallrye / jandex

Java Annotation Indexer
Apache License 2.0
398 stars 94 forks source link

Add convenient builders for most org.jboss.jandex.Type implementations #294

Closed mkouba closed 1 year ago

Ladicek commented 1 year ago

So this PR adds a bunch of factory methods, and also a bunch of shallow builders. By "shallow builder", I mean the builder doesn't help in creating the entire type structure -- for each type in the structure, you need to create it separately. That is, the builder only adds one thing on top of the factory methods -- the ability to set type annotations. Do you have any use case for that? If not, I'd rather settle with the factory methods.

EDIT: I mean, I don't think we need to allow users to create types with type annotations. I have yet to see a need for that.

EDIT 2: I also don't see a need for factory methods for TypeVariable or WildcardType, but I can imagine use cases for that.

mkouba commented 1 year ago

So this PR adds a bunch of factory methods, and also a bunch of shallow builders. By "shallow builder", I mean the builder doesn't help in creating the entire type structure -- for each type in the structure, you need to create it separately.

I don't think that it's a problem. You can add a bunch of specialized methods but the builder class would grow a lot and the benefits are disputable.

That is, the builder only adds one thing on top of the factory methods -- the ability to set type annotations.

Well, I believe that a builder should make it possible to construct a complete class instance, i.e. to specify all attributes. And that's the reason why I added the type annotations.

I don't have a use case now but that does mean there is none. And given the fact that adding this functionality is very "cheap" I don't see a reason why not :shrug:

Do you have any use case for that? If not, I'd rather settle with the factory methods.

Factory methods are great for simple stuff but builders are much more flexible, readable and also exensible. It's IMO better to have a builder rather than a ton of factory methods to cover all possible combinations of parameters.

EDIT: I mean, I don't think we need to allow users to create types with type annotations. I have yet to see a need for that.

EDIT 2: I also don't see a need for factory methods for TypeVariable or WildcardType, but I can imagine use cases for that.

A typical use case are arguments of a parameterized type: List<T> , Map<String, ?> or even List<Map<String, ? extends Serializable>>. You could either add special methods to the builder or use factory methods/"shallow" builders.

I personally find the latter much more readable and more flexible.

That said, we could have both factory methods and builders. And even evolve the builders in the future. That would be 10x better than what we have right now.

Ladicek commented 1 year ago

That's fair enough. We can have builders to allow constructing everything possible, and factory methods for the simple cases. Which means I'd probably get rid of factory methods for TypeVariable and WildcardType and leave just the builders.

mkouba commented 1 year ago

Which means I'd probably get rid of factory methods for TypeVariable and WildcardType and leave just the builders.

Why? TypeVariable.create("T") is IMO just ok. And WildCard#create(Type bound, boolean isExtends) is there since 2.1 so we should not remove it (maybe deprecate). And maybe even add WildCard.upperBound() and WildCard.lowerBound().

Ladicek commented 1 year ago

Ah I forgot there's a factory for WildcardType already. In that case, well, OK, let's have both.

mkouba commented 1 year ago

An example usage of TypeVariable builder for List<T extends Serializable>:

ParameterizedType.create(List.class, TypeVariable.builder("T").addBound(Serializable.class).build())

For WildcardType the factory methods createUpperBound() and createLowerBound() make IMO a lot of sense:

An example usage for List<? super Number>:

ParameterizedType.create(List.class, WildcardType.createLowerBound(Serializable.class))

The builder is there just for the future use ;-). I've also added org.jboss.jandex.WildcardType.UNBOUNDED which represents ?.

Ladicek commented 1 year ago

Thank you, and sorry for all the trouble I caused by merging a conflicting PR first! :heart_decoration: :joy:

mkouba commented 1 year ago

Thank you, and sorry for all the trouble I caused by merging a conflicting PR first! heart_decoration joy

No problem at all. Thanks!