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

Add option to omit getters/setters on models #2078

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

delenius
Copy link
Contributor

@delenius delenius commented Feb 9, 2016

Adds an option to the JS client generator, omitModelMethods, which causes it to not generate getters and setters for the model classes.

These methods are often not needed or desired, especially when the returned objects are to be further processed and turned into other structures anyway (e.g. Immutable.js records).

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

One way we can leverage the setter method is to support enum values as enum is not natively support in JS.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

@wing328, how do you mean?

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

Here is an example for Python: https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/python/swagger_client/models/pet.py#L194)

This is a "workaround". For JS, there's no private variable in model classes so developers can bypass the setter.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

Oh I see, for input validation in the setters. I didn't consider that since the JS generator doesn't actually do that ;)

Still, my PR adds an option that does not break anything...

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

@delenius yes, I'm aware you're adding an option so it's definitely backward-compatible.

Based on your experience, how do JS developers implement enum? It's not a must that we support enum if it's not something supported by JS natively but it would be nice if the JS models can leverage the information defined in enum in the spec to validate the value.

My suggestion is definitely not ideal as you've pointed out that JS developers do not really use setter on models but worth sharing with you how potentially a setter method can be used to "partially" support enum.

@delenius
Copy link
Contributor Author

delenius commented Feb 9, 2016

Well, my sense is that JS developers are used to living without these kinds of safety belts, and tend to use objects in a more loose, free-wheeling way :)

@wing328
Copy link
Contributor

wing328 commented Feb 9, 2016

@delenius ok. we'll reconsider enum support in JS if there're more demands from JS developers.

@demonfiddler
Copy link
Contributor

Why do we need getXxx() and setXxx() methods anyway? It's not a JavaScript idiom and in any case it's superfluous since ECMAScript 5.1 allows for get, set property accessor functions so that client code can use direct property reference rvalues and lvalues respectively.

@wing328
Copy link
Contributor

wing328 commented Feb 10, 2016

@delenius please rebase on the latest master to resolve the merge conflicts.

@delenius
Copy link
Contributor Author

@wing328 - I've fixed it.

@wing328
Copy link
Contributor

wing328 commented Feb 11, 2016

@delenius thanks for the quick turnaround. PR merged into master

wing328 added a commit that referenced this pull request Feb 11, 2016
Add option to omit getters/setters on models
@wing328 wing328 merged commit fdaf1e6 into swagger-api:master Feb 11, 2016
@delenius delenius deleted the omit-model-methods branch February 11, 2016 01:31
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.

3 participants