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

chore: add additionalProperties preset hooks #71

Conversation

jonaslagoni
Copy link
Sponsor Member

@jonaslagoni jonaslagoni commented Feb 18, 2021

Description
This PR adds additionalProperties preset hooks.

Related issue(s)
solves #68

@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented Feb 18, 2021

@magicmatatjahu how should we handle additionalProperties in Javascript renderer? 🤔

@jonaslagoni jonaslagoni changed the title chore: add basic rendering of additional properties chore: add basic rendering of additionalProperties Feb 19, 2021
@jonaslagoni jonaslagoni marked this pull request as ready for review February 24, 2021 09:35
@jonaslagoni
Copy link
Sponsor Member Author

@magicmatatjahu I have fully added the typescript rendering can you check it out in regards to presets and the setup there? Also the class encapsulation for the additionProperties should they have presets?

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Good, but I have an another idea how to use additionalProperties :) It's a more generic.

@@ -77,6 +77,11 @@ ${lines.map(line => ` * ${line}`).join('\n')}
const rendererProperty = await this.runPropertyPreset(propertyName, property, this.model);
content.push(rendererProperty);
}

if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
if (this.model.additionalProperties instanceof CommonModel) {

@@ -52,6 +52,12 @@ ${this.indent(this.renderBlock(content, 2))}
content.push(this.renderBlock([getter, setter]));
}

if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
if (this.model.additionalProperties instanceof CommonModel) {

Copy link
Member

Choose a reason for hiding this comment

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

Like in TS -> to additionalContent preset

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

What do you mean by this? I don't use additionalContent in TypeScript

Copy link
Member

Choose a reason for hiding this comment

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

I mean that this part should be moved to additionalContent preset like I suggest in another comment for TS :)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I guess you mean this for JS - #71 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry 😄

Comment on lines 24 to 31
export interface AdditionalPropertiesArgs {
parentModel: CommonModel;
additionalProperties: CommonModel;
}
export interface ClassPreset<R extends AbstractRenderer, O extends object = any> extends CommonPreset<R, O> {
ctor?: (args: PresetArgs<R, O>) => Promise<string> | string;
property?: (args: PresetArgs<R, O> & PropertyArgs) => Promise<string> | string;
additionalProperties?: (args: PresetArgs<R, O> & AdditionalPropertiesArgs) => Promise<string> | string;
Copy link
Member

Choose a reason for hiding this comment

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

additionalProperties should have only PresetArgs arguments, why? parentModel is not needed - it's my fault and in next PR I will remove it - we have model property for that (it is the same 😅). Also additionalProperties field is hardcoded for additionalProperties inferred from model so you have also possibility to use it by model. additionalProperties preset should have also possibility to render patternProperties etc, so hardcoded for only one field is very bad idea.

additionalProperties?: (args: PresetArgs<R, O>) => Promise<string> | string;

src/models/Preset.ts Outdated Show resolved Hide resolved
src/generators/typescript/TypeScriptRenderer.ts Outdated Show resolved Hide resolved
@@ -77,6 +77,11 @@ ${lines.map(line => ` * ${line}`).join('\n')}
const rendererProperty = await this.runPropertyPreset(propertyName, property, this.model);
content.push(rendererProperty);
}

if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
const additionalProperty = await this.runAdditionalPropertiesPreset(this.model.additionalProperties, this.model);
Copy link
Member

@magicmatatjahu magicmatatjahu Feb 24, 2021

Choose a reason for hiding this comment

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

additionalProperties preset should also render another properties like patternProperties - also passed by options to preset - so this part should look like:

// from this
if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
  const additionalProperty = await this.runAdditionalPropertiesPreset(this.model.additionalProperties, this.model);
  content.push(additionalProperty);
}

// to 
const additionalProperties = await this.runAdditionalPropertiesPreset();
if (additionalProperties) {
  content.push(additionalProperties);
}

we shouldn't hardcoded only for additionalProperties from model.

Comment on lines +39 to +44
if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
const getter = 'getAdditionalProperty(key){ return _additionalProperties[key]; }';
const setter = 'setAdditionalProperty(key, value) { _additionalProperties[key] = value; }';
content.push(this.renderBlock([getter, setter]));
}

Copy link
Member

Choose a reason for hiding this comment

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

it should be rendered inside additionalContent preset:

{
  class: {
    additionalContent({ renderer, model, content }) {
      if (!(this.model.additionalProperties instanceof CommonModel)) return content;
    
      const getter = 'getAdditionalProperty(key){ return _additionalProperties[key]; }';
      const setter = 'setAdditionalProperty(key, value) { _additionalProperties[key] = value; }';
      this.renderBlock([content, getter, setter]);
     }
  }
}

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I think you mean the extra functions that are not really encapsulations right? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for that additionalContent exists - for extra function. Encapsulation is a problem, but you can create separate presets to make your encapsulation

new TypeScriptGenerator({ presets: [ADDITIONALPROPETIES, PATTERN_PROPERTES] })

Copy link
Member

Choose a reason for hiding this comment

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

We can make separate presets for everything, argument, type signature etc. but then it would be hard to use.

@@ -36,6 +36,11 @@ ${content}
content.push(rendererProperty);
}

if (this.model.additionalProperties !== undefined && this.model.additionalProperties instanceof CommonModel) {
Copy link
Member

@magicmatatjahu magicmatatjahu Feb 24, 2021

Choose a reason for hiding this comment

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

Please add it like in JS -> to additionalContent preset

jonaslagoni and others added 2 commits February 24, 2021 12:38
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
@jonaslagoni jonaslagoni changed the title chore: add basic rendering of additionalProperties chore: add additionalProperties preset hooks Feb 25, 2021
…onalProperties_rendering

# Conflicts:
#	src/generators/typescript/TypeScriptRenderer.ts
#	src/generators/typescript/renderers/ClassRenderer.ts
#	test/generators/typescript/TypeScriptGenerator.spec.ts
@jonaslagoni jonaslagoni marked this pull request as draft April 26, 2021 09:22
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.9% 0.9% Duplication

@jonaslagoni
Copy link
Sponsor Member Author

Gonna provide a clean implementation here, gonna use most of the comments already given to solve it. Thanks for the review @magicmatatjahu 🙇

@jonaslagoni jonaslagoni closed this Jun 7, 2021
@jonaslagoni jonaslagoni deleted the feature/additionalProperties_rendering branch June 7, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants