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

improve extension API documentation and resolve some open questions #515

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Aug 19, 2021

  • renamed/moved AnnotationInfo.MEMBER_VALUE to AnnotationMember.VALUE
  • replaced the phrase "component type" with "element type" when speaking
    about arrays, because JLS uses both and each has a different meaning
  • removed ContextConfig, custom context registration is now fully
    handled in MetaAnnotations
  • removed unnecessarily concrete language from @SkipIfPortableExtensionPresent
  • methods in the Types class now have more specific return types
  • implementations of AnnotationInfo, AnnotationMember and AnnotationTarget
    are now required to define equals and hashCode, and are explicitly
    not required to use a single object to represent a single construct
  • the ClassInfo declaration is now required to return annotations
    @Inherited from a superclass, per @Inherited spec and CDI spec
  • explicitly documented that ClassInfo doesn't provide access to nested
    classes or an enclosing class, and PackageInfo doesn't provide access
    to package members
  • corrected some language to distinguish between generic classes (before
    type argument application) and parameterized types (after type argument
    application)
  • specified meaning of MethodInfo.name and returnType for constructors,
    mirroring what reflection does (returning the declaring class)
  • defined how implicit bounds are represented in TypeVariable
    and WildcardType
  • removed ObserverInfo.id()
  • usage of Optional was eliminated everywhere
  • instead of "void type", we now always use "void pseudo-type"
  • added @since 4.0 tags everywhere
  • changed @link tags to @linkplain if the link text isn't an identifier

@Ladicek Ladicek requested a review from graemerocher August 19, 2021 14:27
@graemerocher
Copy link
Contributor

Hey @Ladicek currently on vacation until 30th of August. Will review when I get back!

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 20, 2021

Sure, no worries, and enjoy! :-)

- renamed/moved `AnnotationInfo.MEMBER_VALUE` to `AnnotationMember.VALUE`
- replaced the phrase "component type" with "element type" when speaking
  about arrays, because JLS uses both and each has a different meaning
- removed `ContextConfig`, custom context registration is now fully
  handled in `MetaAnnotations`
- removed unnecessarily concrete language from `@SkipIfPortableExtensionPresent`
- methods in the `Types` class now have more specific return types
- implementations of `AnnotationInfo`, `AnnotationMember` and `AnnotationTarget`
  are now required to define `equals` and `hashCode`, and are explicitly
  not required to use a single object to represent a single construct
- the `ClassInfo` declaration is now required to return annotations
  `@Inherited` from a superclass, per `@Inherited` spec and CDI spec
- explicitly documented that `ClassInfo` doesn't provide access to nested
  classes or an enclosing class, and `PackageInfo` doesn't provide access
  to package members
- corrected some language to distinguish between generic classes (before
  type argument application) and parameterized types (after type argument
  application)
- specified meaning of `MethodInfo.name` and `returnType` for constructors,
  mirroring what reflection does (returning the declaring class)
- defined how implicit bounds are represented in `TypeVariable`
  and `WildcardType`
- removed `ObserverInfo.id()`
- usage of `Optional` was eliminated everywhere
- instead of "`void` type", we now always use "`void` pseudo-type"
- added `@since 4.0` tags everywhere
- changed `@link` tags to `@linkplain` if the link text isn't an identifier
* @throws IllegalArgumentException if the {@code scopeAnnotation} isn't meta-annotated {@code @NormalScope}
* or {@code @Scope}
*/
void addContext(Class<? extends Annotation> scopeAnnotation, Class<? extends AlterableContext> contextClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add variants that take a ClassInfo for contextClass since this is likely to be part of the implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point (@Discovery), there's no way how to obtain a ClassInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I can think of is we can change that parameter to String, if Class is bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok leave it as is

// to the numeric primitive types and maybe String. I currently left the numeric
// as* methods documented as coercing, while asString as not coercing, but this
// needs more discussion. I personally don't like coercion here and would always
// throw if the type mismatches.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems it is a slippery slope and just disallowing coercion may be easier to reason about. Having said that my current implementation is doing coercion

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'd prefer disallowing coercion, but again, I can be convinced otherwise :-)


// TODO what about constructors?
/**
* Returns the name of this method. In case of constructors, returns the binary name of the declaring class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it not return <init> for constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could do both. I thought class name would be better, as that's what reflection does, but I can be convinced otherwise.

@Ladicek Ladicek merged commit d04e0e1 into jakartaee:master Aug 30, 2021
@Ladicek Ladicek deleted the api-improvements branch August 30, 2021 09:28
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2021

Thanks!

@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