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 new generators. #15883

Merged
merged 10 commits into from
Aug 9, 2021
Merged

Improve new generators. #15883

merged 10 commits into from
Aug 9, 2021

Conversation

mshima
Copy link
Member

@mshima mshima commented Aug 7, 2021


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mshima mshima requested a review from Tcharl August 7, 2021 10:55
@Tcharl
Copy link
Contributor

Tcharl commented Aug 7, 2021

Way cleaner than what we were before for sure :-). Perfect!

generators/gradle/constants.cjs Show resolved Hide resolved
generators/spring-boot/mixin.cjs Outdated Show resolved Hide resolved
generators/spring-boot/mixin.cjs Show resolved Hide resolved
registerSpringBootOptions() {
this.jhipsterOptions(options);
getSpringBootOptions() {
return options;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it the occasion to set scopes for options? ApplicationScope (or platformscope), ModuleScope, EntityScope and entityKeyScope?

Copy link
Member Author

@mshima mshima Aug 8, 2021

Choose a reason for hiding this comment

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

They cannot mix with applications and I hope mixins will not be required for entities.
What do you consider a platform scope?
From my point of view, it's specific to a technology, and are hierarchical instead of polymorphic.

By changing getSpringBootOptions to getSpringBootApplicationOptions, it would make sense to change the entire mixin api to *Application*.

Copy link
Contributor

@Tcharl Tcharl Aug 8, 2021

Choose a reason for hiding this comment

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

Monitoring and service registry? I think that also docker-compose, kubenretes, openshift, ... Any platform configuration could be part of the platformconfig.
And very good idea for the Application mixins ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

We should create platform generator with those configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but in addition I was thinking about populating a dedicated scope object for each kind of options instead of injecting them in 'this'.

@mshima mshima merged commit d50a515 into jhipster:main Aug 9, 2021
@mshima mshima deleted the skip_ci-priorities branch August 9, 2021 14:13
@pascalgrimaud pascalgrimaud added this to the 7.2.0 milestone Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants