Convert templates in packages #219
Conversation
There was a problem hiding this comment.
We do not validate if provided template exists and now problem is that when I do node cli.js -o ./output ./test/docs/streetlights.yml test new dir in provided or default templates locaton is created with package-lock.json file
{
"lockfileVersion": 1
}
It is not fault of installTemplate function but simply because we do not have the nonexisting template validation. If I'm getting the basics of the generator correct so far, the needed validation would only have to: check if in a given templates directory, a directory with given template name exists (test in my case) and if it contains .tp-config.json file.
Most important is though that nodejs template works like a charm with Node.js v13.7.0
And also I think we do not really need lerna for packages installation thanks to npmi
README.md
Outdated
| -d, --disable-hook <hookName> disable a specific hook | ||
| -n, --no-overwrite <glob> glob or path of the file(s) to skip when regenerating | ||
| -p, --param <name=value> additional param to pass to templates | ||
| -t, --templates <templateDir> directory where templates are located (default: Internal template folder) | ||
| -t, --templates <templateDir> directory where templates are located (defaults to internal templates directory) | ||
| --force-install forces the installation of the template dependencies |
There was a problem hiding this comment.
I added those suggestions to explain the flag as at first it was weird for me that there is a --force-install flag but I did not pass it and dependencies were installed anyway (at first run). Maybe the name is confusing, maybe reinstall or something 🤔
btw, why no -f flag?
There was a problem hiding this comment.
The -f flag will be a generic --force flag. It would "force" everything. Including, for instance, the generation when the repo has unstaged changes. It's useful but it's probably another flag and another PR.
There was a problem hiding this comment.
yeap, since last release we have --force-write so --force-install now nicely adds to the list and then as you wrote we can have -f god to apply all force flags
| @@ -485,6 +490,8 @@ class Generator { | |||
| const targetFile = path.resolve(this.targetDir, relativeSourceFile); | |||
| const relativeTargetFile = path.relative(this.targetDir, targetFile); | |||
|
|
|||
| if (shouldIgnoreFile(relativeSourceFile)) return; | |||
There was a problem hiding this comment.
please add code comment here what we are protecting from here as I couldn't figure out why,
I ran all templates and was never able to get true for any file. I see proper protection is here https://github.com/asyncapi/generator/blob/master/lib/generator.js#L379
There was a problem hiding this comment.
This is not the only place where generateFile is called, that's why. I should probably remove it from the line you mentioned.
Co-Authored-By: Lukasz Gornicki <[email protected]>
Co-Authored-By: Lukasz Gornicki <[email protected]>
|
This PR is of your interest, @damaru-inc. |
|
|
Good catch. It's fixed now. Regarding the "defaults", I unified everything except the |
|
Would be almost super cool but I have to be picky on this error message that we show when I provide a name of the template that doesn't exist: Code behind this error message only checks if directory exists and not if it is a template. If I would have a directory with 2 options here:
2 is better but I can live with 1 too :) |
|
Implemented 2. However, the |
|
LGTM. Awesome stuff!!! merge and release and let us go forward with rest of improvements |
|
Thanks for the great review mate! |
Fixes #214
Fixes #198
TODO