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

Finalize unambiguous priorities api changes. #19978

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

mshima
Copy link
Member

@mshima mshima commented Oct 10, 2022

  • change every generator to extend base/index.cjs instead of generator-base-blueprint or generator-base.
    generator-base-blueprint and generator-base are in the inheritance tree, but should be dropped for v8.
  • complete unambiguous api changes.
  • drop deprecated preparingFields and preparingRelationships priorities
  • cleanup a few unused generator-base methods.
  • move lib/constants/priorities to near the generator that implements it.

Closes #17132


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

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

PRIORITY_NAMES.PREPARING,
CONFIGURING_EACH_ENTITY,
LOADING_ENTITIES,
PREPARING_EACH_ENTITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

The EACH term is a bit confusing, it could mean that it will modify EACH Entities ^^.

Removing the EACH looks to be more meaningful (meaning that we are focusing on one specific entity).
Am I wrong with the meaning? If it really means EACH, I would go for PREPARING_ENTITIES.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can rename, but let's do it in another PR.

@@ -0,0 +1,138 @@
const { PRIORITY_PREFIX, QUEUE_PREFIX, PRIORITY_NAMES, QUEUES } = require('../base/priorities.cjs');
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 worth it to create a base-platform/priorities.cjs (a bunch of application), a base-entity/priorities.cjs and maybe a base-field/priorities.cjs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it worth it to create a base-platform/priorities.cjs (a bunch of application).

Yes, I think we should call it base-workspaces.
I will create a feature request.

a base-entity/priorities.cjs and maybe a base-field/priorities.cjs?

I don't think so.
But I think we should implement a base-incremental-application for entity changes like liquibase and maybe entity removal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some users are complaining that generation is all or nothing. Splitting metamodels (and models) could ease to generate only entities and fields that have been updated

Copy link
Member Author

Choose a reason for hiding this comment

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

@Tcharl the application, entities, fields and relationships are dependent on each other, it's not possible to create a consistent application without preparing everything.
See #19989.

@@ -508,7 +508,7 @@ module.exports = class JHipsterServerGenerator extends BaseApplicationGenerator
}

get [BaseApplicationGenerator.CONFIGURING_EACH_ENTITY]() {
return this.asPromptingTaskGroup(this.delegateToBlueprint ? {} : this.configuringEachEntity);
return this.asConfiguringEachEntityTaskGroup(this.delegateToBlueprint ? {} : this.configuringEachEntity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem: asConfiguringEntityTaskGroup

@Tcharl
Copy link
Contributor

Tcharl commented Oct 10, 2022

Reviewed
Please take the comments into consideration:

  • Priority names should be crystal clear, the X_EACH_ENTITY is not that clear: we don't know if the processing should be on every entities (in this case X_ENTITIES would be more clear) or if the processing is applied on one specific entity (thus X_ENTITY would be more clear)
  • It may be cleaner to split the generator's lifecycle (and metamodel) of platform vs application vs entity vs fields. It would prepare future 'thiner' generators and improve the readability of the model and generator's code

@mshima mshima changed the title Finalize priorities api changes. Finalize unambiguous priorities api changes. Oct 11, 2022
@mshima
Copy link
Member Author

mshima commented Oct 11, 2022

@Tcharl can this PR be merged?
It only reorganizes current implementation, and cleanup some methods.
If we need to change anything we should create a new PR.

@mraible
Copy link
Contributor

mraible commented Oct 11, 2022

I took a brief look and it seems like most phases are suffixed with 'ing', which makes me think they are called at the beginning of a phase. The only one that looks weird is "END", which might make sense if it's called at the very end.

@mshima mshima merged commit 632d62b into jhipster:main Oct 12, 2022
@mshima mshima deleted the skip_ci-unambiguous branch October 12, 2022 15:17
@DanielFran DanielFran added this to the 8.0.0-beta.1 milestone Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unambiguous Priorities
4 participants