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

remove App{Archive,ArchiveConfig,Deployment}, rename AppArchiveBuilder and ContextBuilder #506

Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jul 27, 2021

The AppArchive, AppArchiveConfig and AppDeployment types
were redundant. Moreover, they looked like they can access whole
world, which prevents certain kinds of implementations. (Some
effort was made to design the API in these types so that whole
world access is not necessary, but it's a leaky abstraction.)
For those reasons, these types are removed altogether.

The AppArchiveBuilder type is renamed to ScannedClasses,
which is more descriptive (and shorter). The addSubtypesOf
method is removed, because it can't be implemented in certain
environments (notably, Portable Extensions).

The ContextBuilder type is renamed to ContextConfig, because
it isn't really used in the usual builder way and is quite close
to the other existing *Config types.

Additionally, some documentation is improved, especially for
types in the @Discovery phase.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 27, 2021

This is just a first round of improvements I have in mind. More will follow for sure.

Since @graemerocher started improving the language model part, I started looking at the actual extension API, so that we don't clash (hopefully :-) ).

/**
* Registers custom context as configured by the returned {@link ContextConfig}.
*/
ContextConfig addContext();
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, maybe we should just have addContext(Class<? extends AlterableContext>) (which is the only mandatory part) and leave ContextConfig purely for the optional stuff?

@Ladicek Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Jul 27, 2021
@Ladicek Ladicek force-pushed the build-compatible-extensions-improvements branch 2 times, most recently from 88f492d to 7197328 Compare July 27, 2021 13:51
…r and ContextBuilder

The `AppArchive`, `AppArchiveConfig` and `AppDeployment` types
were redundant. Moreover, they looked like they can access whole
world, which prevents certain kinds of implementations. (Some
effort was made to design the API in these types so that whole
world access is not necessary, but it's a leaky abstraction.)
For those reasons, these types are removed altogether.

The `AppArchiveBuilder` type is renamed to `ScannedClasses`,
which is more descriptive (and shorter). The `addSubtypesOf`
method is removed, because it can't be implemented in certain
environments (notably, Portable Extensions).

The `ContextBuilder` type is renamed to `ContextConfig`, because
it isn't really used in the usual builder way and is quite close
to the other existing `*Config` types.

Additionally, some documentation is improved, especially for
types in the `@Discovery` phase.
@Ladicek Ladicek force-pushed the build-compatible-extensions-improvements branch from 7197328 to deb6d74 Compare July 27, 2021 14:14
@Ladicek Ladicek merged commit 0c99dfc into jakartaee:master Jul 29, 2021
@Ladicek Ladicek deleted the build-compatible-extensions-improvements branch July 29, 2021 08:46
@graemerocher
Copy link
Contributor

Great this makes life much simpler. You working the AnnotationBuilder PR now?

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 29, 2021

I do, yes. Should be up shortly -- I have to resist the urge to change too many things at once :-)

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.

None yet

2 participants