-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Move common functionality to new model base class #2075
Conversation
We've similar model implementation (refactor model functions into a base class) for Ruby but we later remove that. The reason is that we want to avoid extra dependency on the model as there are use cases just use the models (not API class) only. |
I don't understand. This does not depend on an "API class" (though I called the base class, perhaps poorly, |
Can the model class be used without In other words, we want to make the model class "self-contained" without dependency on another base class. |
No, the model classes are not self-contained in that sense. However, the model classes can already depend on other model classes when you have fields of some user-defined type. Another potential need for inheritance in the model classes is to support polymorphism properly (see #2041). In short, there is nothing wrong with having model classes depend on other model classes, and in fact you already do this. |
There's definitely nothing wrong and as I said we did it before (with the exact reason you mentioned in this PR). I just wanted to share the reason with you why we rolled back to the original approach. |
Fixes "additional comment swagger-api#2" in swagger-api#2044. Saves memory by not repeating the `toJson` method in every model class. The new `ApiModel` base class may also be useful for other purposes in the future.
f0af196
to
5e25c66
Compare
@wing328 since you don't want to do the refactoring above, how do you feel about simply removing the {{classname}}.prototype.toJson = function() {
return JSON.stringify(this);
} It seems more straightforward to just using |
@delenius once again thanks for this PR. I was really happy when I saw it as you took a huge step further to bring the JS API client to the next level. I agree that the one-liner (using JSON.stringify) should be pretty straightforward to most JS developers and one should also easily find the solution by doing a google search (example). One question I always consider is can a 10-year-old who just learns JS programming for 3 months use the JS API client generated by swagger-codegen? would the Of course, another view we also need to consider is whether the I've shared my view and if you want to remove |
I'll hold off for a little while to see if we get any tie-breaker opinion on this :) |
I'm at a loss to understand what the to Surely standard serialisation via Regarding a generic base class for all models, this would be useful if and only if we had something useful to put in it. On a previous EMF-to-JavaScript project I did use a generic base class to implement the 'pure modelling' semantics of containment, bidirectional associations, runtime type safety, etc. and I suppose that at some future point we might entertain such capabilities. Apart from that, it's possible that an abstract base model class might be useful to support polymorphism - see #2041. The generated X.constructFromObject() method would need to inspect the discriminator field to determine which sub-class to instantiate - such logic could be placed in the base model class to avoid replicating it in every (extended?) class. |
Fixes "additional comment (2)" in #2044.
The
toJson
method that currently exists (identically) in every model class is moved to a common base class,ApiModel
, that is extended by all model classes (using prototype inheritance).The new
ApiModel
base class may also be useful for other purposes (i.e. other common functionality among the model classes) in the future.Note: This also includes the changes to the Petstore example files from #2071 because it would be impossible not to (since I had to regenerate the example files using the latest code).
A couple of debatable points:
ApiModel
. Perhaps you preferBaseModel
,ModelBase
, or justModel
?model
dir. Currently it goes whereApiClient.js
goes.