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

extension API improvements #511

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Aug 4, 2021

That is specifically:

  • change the *Config types to no longer inherit from the corresponding
    *Info types; instead, they provide an info() method
  • rename AnnotationConfig to DeclarationConfig for symmetry
    between *Info and *Config types
  • change the annotation transformation methods on DeclarationConfig
    to return this to allow fluent usage
  • allow additional parameter types for @Enhancement methods:
    DeclarationConfig, DeclarationInfo, ClassInfo, MethodInfo,
    and FieldInfo (*Info types are useful when only collecting
    information during @Enhancement, not modifying anything)
  • remove useless type parameters from AnnotationInfo, ClassInfo,
    MethodInfo, FieldInfo, ClassConfig, MethodConfig, FieldConfig,
    BeanInfo, and ObserverInfo; the SyntheticBeanBuilder type
    still has a type parameter, which remains an open question
  • add a lot of documentation

@Ladicek Ladicek requested a review from graemerocher August 4, 2021 13:11
* @return a collection of {@link MethodConfig} objects, never {@code null}
*/
// TODO specify whether inherited constructors are also included
Collection<MethodConfig> constructors();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No these should just be the declared constructors, inherited ones can be obtained by traversing the super class?

* @return a collection of {@link MethodConfig} objects, never {@code null}
*/
// TODO specify whether inherited methods are also included
Collection<MethodConfig> methods();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it may make sense to have declaredMethods() vs methods()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is something I'm a little ambivalent about. I think we already discussed something similar for annotations and kinda agreed that we'll always return all annotations, including inherited ones. We should probably do the same here.

I'm looking at what Weld does for Portable Extensions (because that seems quite under-documented on the Portable Extensions API side), and it works like this:

  • for constructors, returns only constructors declared on this class
  • for fields, returns all fields declared on this class and all its superclasses up to and excluding Object (which also includes private fields from superclasses)
  • for methods, returns all methods declared on this class and all its superclasses up to and excluding Object (which also includes private methods from superclasses), plus also all default methods from all interfaces directly and indirectly implemented by this class

I guess that kinda sorta makes sense. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense for these methods yes. Although they will be relatively expensive methods to call since materializing all that info could be costly.

Micronaut has an extensive querying API for methods/fields that lets you filter methods prior to materializing the actual equivalent of MethodConfig https://github.com/micronaut-projects/micronaut-core/blob/3.0.x/inject/src/main/java/io/micronaut/inject/ast/ElementQuery.java

I am not sure if something like this makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking about it and I've got two little problems with methods from interfaces. We would include abstract methods from [super]classes, but not from [super]interfaces. Also, we would include all default methods, even if they potentially clash. It still kinda makes sense, but feels arbitrary. I'll think a bit more about it.

The situation for constructors and fields is rather clear, so I'll draft something and amend the PR.

Also, we already had a bit of querying API in AppArchive and AppDeployment. We might be able to bring some of that back for sure, but I don't think this is too big of an issue. Implementations should be able to materialize the collection lazily and memoize the result.

@Ladicek Ladicek force-pushed the api-improvements branch 2 times, most recently from 15d42ac to e6d7307 Compare August 5, 2021 09:45
That is specifically:

- change the `*Config` types to no longer inherit from the corresponding
  `*Info` types; instead, they provide an `info()` method
- rename `AnnotationConfig` to `DeclarationConfig` for symmetry
  between `*Info` and `*Config` types
- change the annotation transformation methods on `DeclarationConfig`
  to return `this` to allow fluent usage
- allow additional parameter types for `@Enhancement` methods:
  `DeclarationConfig`, `DeclarationInfo`, `ClassInfo`, `MethodInfo`,
  and `FieldInfo` (`*Info` types are useful when only collecting
  information during `@Enhancement`, not modifying anything)
- remove useless type parameters from `AnnotationInfo`, `ClassInfo`,
  `MethodInfo`, `FieldInfo`, `ClassConfig`, `MethodConfig`, `FieldConfig`,
  `BeanInfo`, and `ObserverInfo`; the `SyntheticBeanBuilder` type
  still has a type parameter, which remains an open question
- add a lot of documentation
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 5, 2021

I just added some javadoc to ClassInfo.constructors(), methods() and fields(), the rest is unchanged.

@Ladicek Ladicek merged commit 483ad63 into jakartaee:master Aug 5, 2021
@Ladicek Ladicek deleted the api-improvements branch August 5, 2021 13:54
@Ladicek Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants