angular generation: Add new additionalProperty - specVersion. Used to…#326
angular generation: Add new additionalProperty - specVersion. Used to…#326
Conversation
… link YAML sepec versin used to generate the NPM package
brianvabb
requested review from
MarcelHeemskerk,
bartveenstra,
daiscog,
ngohungphuc and
santam85
| "name": "{{{npmName}}}", | ||
| "version": "{{{npmVersion}}}", | ||
| "description": "OpenAPI client for {{{npmName}}}", | ||
| "specVersion": "{{{specVersion}}}", |
There was a problem hiding this comment.
Should we make this conditional so that it only appears if it has a value?
Also do we want to put this inside a backbase object to avoid potential clashes if package.json adds an official specVersion property in the future for some purpose?
There was a problem hiding this comment.
Some test failures, will take another look ^
| "version": "{{{npmVersion}}}", | ||
| "description": "OpenAPI client for {{{npmName}}}", | ||
| "backbase": { | ||
| "specVersion": "{{{specVersion}}}" |
There was a problem hiding this comment.
- let's make it conditional here too
| }); | ||
|
|
||
| processOpt(SPEC_VERSION, | ||
| value -> additionalProperties.put(SPEC_VERSION, new SemVer(value)), |
There was a problem hiding this comment.
Here is where it's trying to parse it as a semver (the new SemVer(value). You could have an if wrapping all of this that only attempts to set the property only if !Strings.isEmpty(value) (which I think is nullsafe)
| false)); | ||
| this.cliOptions.add(new CliOption(NG_VERSION, "The version of Angular. (At least 10.0.0)").defaultValue(this.ngVersion)); | ||
| this.cliOptions.add(new CliOption(FOUNDATION_VERSION, "The version of foundation-ang library.").defaultValue(this.foundationVersion)); | ||
| this.cliOptions.add(new CliOption(SPEC_VERSION, "The version of OpenAPI YAML spec used to generate the NPM package.").defaultValue(this.specVersion)); |
There was a problem hiding this comment.
We should remove the default value, otherwise making it conditional in the mustache template has no effect.
| false)); | ||
| this.cliOptions.add(new CliOption(NG_VERSION, "The version of Angular. (At least 10.0.0)").defaultValue(this.ngVersion)); | ||
| this.cliOptions.add(new CliOption(FOUNDATION_VERSION, "The version of foundation-ang library.").defaultValue(this.foundationVersion)); | ||
| this.cliOptions.add(new CliOption(SPEC_VERSION, "The version of OpenAPI YAML spec used to generate the NPM package.").defaultValue(this.specVersion)); |
There was a problem hiding this comment.
@ngohungphuc @santam85 this cli option should be included right? So devs know what the property is used for. Just need to remove the .defaultValue(this.specVersion)); part so it's not assigned a default value.
|
|
||
| processOpt(SPEC_VERSION, | ||
| value -> { | ||
| if(!Strings.isEmpty(value)) { |
There was a problem hiding this comment.
Looking at the build checks, seems there is something wrong with this function syntax? - https://github.com/Backbase/backbase-openapi-tools/runs/8110552249?check_suite_focus=true
There was a problem hiding this comment.
Yeah you're right. I should've used the StringUtils
|
Kudos, SonarCloud Quality Gate passed! |
| public static final String HAS_EXAMPLES = "hasExamples"; | ||
| public static final String PATTERN = "pattern"; | ||
| protected String foundationVersion = "6.6.7"; | ||
| protected String specVersion = "1.0.0"; |
There was a problem hiding this comment.
Since this var is being initialised to "1.0.0" will the specVersion also have a default? I think we don't wan't to give it a default so it will be omitted from the package.json if not provided in POM properties.
| public static final String HAS_EXAMPLES = "hasExamples"; | ||
| public static final String PATTERN = "pattern"; | ||
| protected String foundationVersion = "6.6.7"; | ||
| protected String specVersion = "1.0.0"; |
There was a problem hiding this comment.
I'm not sure it's a good idea to default to a particular version; it will create incorrect results for people who are not setting this property.
| additionalProperties.put(SPEC_VERSION, new SemVer(value)); | ||
| } | ||
| }, | ||
| () -> { |
There was a problem hiding this comment.
Again, I'm not sure we should do anything at all when the user does not provide a value.
4 participants
Description
Add new additionalProperty -
specVersion. Used to link YAML spec version that was used to generate the Angular NPM packageFixes # nojira
Type of change