simplify git options and config #30229
Conversation
There was a problem hiding this comment.
No so sure about what I did: Options vs config is not so clear to me: from what I've seen, option looks to be a superset of config but I don't get the intrinstic difference: maybe we could get rid of options an treat everything as config anyway (context or storage) or get rid of context and keep a clear separation between option and config (persisting options in config when storage is set and all the rest being part of the option type).
Options is every option relevant to generator it's more relevant to external generators when composing with the generator. It may be transient like skipChecks.
Config are stored inside json, it may be a state like last changelog date.
I will add some jsdoc
| @@ -15,7 +15,6 @@ export type OptionsAll = Simplify< | |||
| ExportGeneratorOptionsFromCommand<typeof import('../../generators/base/command.ts').default> & | |||
| ExportGeneratorOptionsFromCommand<typeof import('../../generators/bootstrap-application-base/command.ts').default> & | |||
| ExportGeneratorOptionsFromCommand<typeof import('../../generators/client/command.ts').default> & | |||
| ExportGeneratorOptionsFromCommand<typeof import('../../generators/git/command.ts').default> & | |||
There was a problem hiding this comment.
There is no reason to remove git from optionsAll
There was a problem hiding this comment.
It's not used anymore in other generators than git (has been move) (considered as resolved: better being part of workspaceoptions)
| @@ -29,6 +29,5 @@ export type ApplicationAll<E extends EntityAll = EntityAll> = BaseApplication<E> | |||
| ClientApplication<E> & | |||
| DockerApplication & | |||
| LiqbuibaseApplication<E> & | |||
| ExportApplicationPropertiesFromCommand<typeof import('../../generators/git/command.ts').default> & | |||
There was a problem hiding this comment.
There is no reason to remove git from propertiesAll
There was a problem hiding this comment.
It's not used anymore in other generators than git (has been move) (considered as resolved: better being part of workspaceoptions)
There was a problem hiding this comment.
All config/properties/application is only used by BaseApplicationGeneratorAll and testing helpers.
helpers.run().withJHipsterConfig() should use ConfigAll.
| @@ -22,7 +22,6 @@ export type ConfigAll = Simplify< | |||
| microfrontends?: { baseName: string }[]; | |||
| } & ExportStoragePropertiesFromCommand<typeof import('../../generators/app/command.ts').default> & | |||
| ExportStoragePropertiesFromCommand<typeof import('../../generators/bootstrap-application-base/command.ts').default> & | |||
| ExportStoragePropertiesFromCommand<typeof import('../../generators/git/command.ts').default> & | |||
There was a problem hiding this comment.
There is no reason to remove git from configAll
There was a problem hiding this comment.
It's not used anymore in other generators than git (has been move to git options) (considered as resolved: better being part of workspaceoptions) : played with inheritance to have workspaceoptions & config in the inheritance chain
There was a problem hiding this comment.
All config/properties/application is only used by BaseApplicationGeneratorAll and testing helpers.
helpers.run().withJHipsterConfig() should use ConfigAll.
| async initializeMonorepository() { | ||
| if (!this.skipGit && this.jhipsterConfig.monorepository) { | ||
| await this.initializeGitRepository(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Monorepository is very hard to handle.
This looks to be wrong.
Monorepository is initialized before composing with applications generators, so generators will detect they are inside a monorepository.
There was a problem hiding this comment.
I've seen that, but is there a need to have git repo initialized twice?
There was a problem hiding this comment.
No, initializeGitRepository will not be executed later in this case.
generators/jdl/types.d.ts
Outdated
| @@ -13,4 +14,4 @@ type JdlOptions = { | |||
|
|
|||
| export type Config = BaseConfig & JdlOptions & Command['Config']; | |||
|
|
|||
| export type Options = BaseOptions & JdlOptions & Command['Options']; | |||
| export type Options = BaseOptions & JdlOptions & Command['Options'] & GitOptions; | |||
There was a problem hiding this comment.
If we are going to expose every Options to JDL generator, then it should be ApplicationAll and DeploymentAll since it will compose with every generator.
JDL types have only types related to JDL generation.
There was a problem hiding this comment.
Git is a bit specific as it is valid for app and deployment :-S, would prefer to keep it here, but the only one in this case (otherwise both workspace and application would be used until jdl merged into config)
There was a problem hiding this comment.
There are 2 options:
- Make git at base option.
- Treat git as a feature that have very specific integration like it's done in https://github.com/jhipster/generator-jhipster/pull/30233/files.
There was a problem hiding this comment.
Did inheritance from workspaceoptions.
Contributes to #28235
No so sure about what I did: Options vs config is not so clear to me: from what I've seen, option looks to be a superset of config but I don't get the intrinstic difference: maybe we could get rid of options an treat everything as config anyway (context or storage) or get rid of context and keep a clear separation between option and config (persisting options in config when storage is set and all the rest being part of the option type).
Please be attentive during review, and give explained feedback. Thank you in advance, as you know I'm eager to simplify the codebase ^^
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-cilabel, you can still see CI build result at your branch.