-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Implement application template context at new generators. #15918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code start to become crystal clear and very efficient!
Thank you very much for that huge improvement!
* Single absolute path or relative path(s) between the templates folder and template path. | ||
* @return {string[]} | ||
*/ | ||
async writeFiles(options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be put in a dedicated files and called from there?
for example:
utils/generation/processing/template-processor.js // for ejs
utils/generation/processing/file-processor.js // for svg, png, or other static files
utils/generation/processing/templates-processor.js // calling the two above
Ideally, if we could make the classes SOLID (including the S) we could get some new opportunity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this will make more complicated.
I will try to make writeFiles more linear.
@@ -52,6 +51,9 @@ module.exports = class extends MixedChain { | |||
|
|||
if (this.options.help) return; | |||
|
|||
// Application context for templates | |||
this.application = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about creating a dedicated object for this?
utils/generation/model/contexts/application-context.js
application = { baseName: undefined, baseFolder; undefined, ...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or in each generator if you prefer to keep them loosely coupled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using javascript not typescript, I don't see the need for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
giving some guidance and a clear contract for anyone which want to know the attributes that are recognized by the generators.
But anyway as you mentioned, javascript is not so pojo oriented...
@@ -58,7 +58,7 @@ module.exports.mixin = parent => | |||
* @param {any} config - config to load config from | |||
* @param {any} into - destination context to use default is context | |||
*/ | |||
loadProjectNameConfig(config = this.jhipsterConfig, into = this) { | |||
loadProjectNameConfig(into = this, config = this.jhipsterConfig) { | |||
config = defaults({}, config, defaultConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config = defaults({}, this.jhipsterConfig, defaultConfig)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want types and objects more strict we should move to typescript otherwise it’s too much development burden for almost no benefits.
* Single absolute path or relative path(s) between the templates folder and template path. | ||
* @return {string[]} | ||
*/ | ||
async writeFiles(options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this will make more complicated.
I will try to make writeFiles more linear.
@@ -52,6 +51,9 @@ module.exports = class extends MixedChain { | |||
|
|||
if (this.options.help) return; | |||
|
|||
// Application context for templates | |||
this.application = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using javascript not typescript, I don't see the need for this.
a42893c
to
d8a39b0
Compare
prepareDerived*Properties
toprepare*DerivedProperties
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.