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

typescript-angular: Feature/model suffix #418

Merged
merged 10 commits into from
Jul 3, 2018

Conversation

tomvangreen
Copy link
Contributor

@tomvangreen tomvangreen commented Jun 29, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I introduced a modelSuffix and modelFileSuffix parameter in order to add suffixes to the generated models and model file names. Taking the samples from the sample, with this setting you could for example change the model Pet in the file pet.ts to PetVm in the file pet.vm.ts by defining the model suffix Vm and the model file suffix .vm.

The motivation behind this change is to have a clear distincation between generated models and models that are created by a developer. Having the ability to add a suffix to the generated models makes it immediatly obvious of what kind a model is.

I currently see one issue and that is that the class suffix gets also appended to enumeration types. The StatusEnum in the Pet class will become StatusVmEnum if I set the modelSuffix option to Vm. I will have a look if I find how I can avoid that.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny

@tomvangreen tomvangreen changed the title Feature/model suffix typescript-angular: Feature/model suffix Jun 29, 2018
@tomvangreen
Copy link
Contributor Author

#234

Copy link
Contributor

@TiFu TiFu left a comment

Choose a reason for hiding this comment

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

How does this handle invalid suffixes? Do we want to ignore this case? (e.g. modelSuffix could make the model class name invalid by using invalid characters)

Looks good otherwise.

@tomvangreen
Copy link
Contributor Author

Yes, you are right. You could set the suffix to something invalid. I think this could also be a problem with the filename.

From what I see we have following options:

  1. Validate and throw an exception if the name is invalid.
  2. Sanitize the input and strip every invalid character and show a warning.
  3. Let it run and fail.

I would say because of the filename probably approach 1 or 2 should be used. Better to catch this problem than to let it fail uncontrolled. Do you know of any similar cases in this project and how it is handled there?

@tomvangreen
Copy link
Contributor Author

I added validation for class and file suffixes. As my earlier change with the service suffix has the same problem, I also added the validation for those configurations.

Currently I only allow alpha numeric characters for class suffixes. For file suffixes I additionally allow the characters '.' and '-'.

If any of the four config values is invalid I throw an IllegalArgument exception and display a message.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

apart from some small documentation improvements this looks good.

}

private void validateFileSuffixArgument(String argument, String value) {
Copy link
Member

Choose a reason for hiding this comment

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

please add a method description. describe what argument and value are or rename them to better understand their meaning

}
}

private void validateClassSuffixArgument(String argument, String value) {
Copy link
Member

Choose a reason for hiding this comment

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

same here

@TiFu
Copy link
Contributor

TiFu commented Jul 2, 2018

Agreed. Thanks for the PR!

@macjohnny
Copy link
Member

@wing328 could you please add this to the 3.1.0 milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants