-
-
Notifications
You must be signed in to change notification settings - Fork 114
Flag to specify whether the @Generated annotation is added #534
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
Flag to specify whether the @Generated annotation is added #534
Conversation
kobylynskyi
left a comment
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.
@streibeb, thanks for the contribution. I think this change may be useful.
In addition to all the comments I made in individual files, could you please add some unit tests?
Thank you!
| generateClient := MappingConfigConstants.DEFAULT_GENERATE_CLIENT, | ||
| generateParameterizedFieldsResolvers := MappingConfigConstants.DEFAULT_GENERATE_PARAMETERIZED_FIELDS_RESOLVERS, | ||
| generateExtensionFieldsResolvers := MappingConfigConstants.DEFAULT_GENERATE_EXTENSION_FIELDS_RESOLVERS, | ||
| addGeneratedAnnotation := MappingConfigConstants.DEFAULT_ADD_GENERATED_ANNOTATION, |
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.
Isn't this duplicated?
| } | ||
| } | ||
| } | ||
| protected final MappingConfig mappingConfig; |
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.
In this repository, we use spaces instead of tabs, sorry 😄
Could you please change only the required lines and use spaces?
Thanks
| private List<ParameterDefinition> getFields(List<ExtendedFieldDefinition> fieldDefinitions, ExtendedDefinition<?, ?> parentDefinition) { | ||
| return this.dataModelMapperFactory.getFieldDefinitionToParameterMapper().mapFields(this, fieldDefinitions, parentDefinition); | ||
| } | ||
| private final MappingConfig config; |
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.
It's hard to tell which changes were made to the class. Could you please use spaces instead of tabs? Thanks.
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.
yes, sorry!
| </#list> | ||
| */ | ||
| </#if> | ||
| <#if generatedAnnotation> |
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.
You can merge two statements:
<#if generatedAnnotation && generatedInfo.getGeneratedType()?has_content>This refers to all .ftl files.
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.
I was looking for the and operator and couldn't find anything about it! Will fix!
kobylynskyi
left a comment
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.
@streibeb, looks good. Thanks for the contribution. I will update codegen-options.md
| generateClient := MappingConfigConstants.DEFAULT_GENERATE_CLIENT, | ||
| generateParameterizedFieldsResolvers := MappingConfigConstants.DEFAULT_GENERATE_PARAMETERIZED_FIELDS_RESOLVERS, | ||
| generateExtensionFieldsResolvers := MappingConfigConstants.DEFAULT_GENERATE_EXTENSION_FIELDS_RESOLVERS, | ||
| addGeneratedAnnotation := MappingConfigConstants.DEFAULT_ADD_GENERATED_ANNOTATION, |
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.
Please use mappingConfig to assign values to each project scope, Otherwise, the configuration is invalid
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.
@jxnu-liguobin could you please make the change? I already merged the PR to master branch. Thank you!
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.
@kobylynskyi The merged code no problem, has been modified
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.
lgtm

Toggle the inclusion of the @generated annotation