search

krakenjs / generator-swaggerize

Yeoman generator for design-driven apis with swagger 2.0 and krakenjs/swaggerize tools.
Other
70 stars 34 forks source link

swaggerize-restify support #33

Closed bardzusny closed 9 years ago

bardzusny commented 9 years ago

Hi,

I forked swaggerize-express and modified it to work with restify - basically made what swagger-node-restify is to swagger-node-express. I would love to get it supported by this generator, and this PR provides just that - any suggestions/comments welcome.

bardzusny commented 9 years ago

I updated PR to use swaggerize-restify 2.x.x - with merged changes from swaggerize-express 4.x.x. The only backwards-incompatible feature I can see, that actually matters in provided with generator-swaggerize app templates, is specyfing API host by setting app.swagger.api.host property instead of using app.setHost() method.

tlivings commented 9 years ago

This is awesome! Will review and comment shortly, thanks.

I apologize that this has sat here so long. To be honest it totally went under my radar, otherwise I would have engaged a lot sooner.

tlivings commented 9 years ago

This looks pretty good to me. My only concern would be keeping things in sync with swaggerize-restify.

bardzusny commented 9 years ago

There are no ambitions above being a mere port of what is already there in swaggerize-express* - I wouldn't worry at all about feature-set "branching away" in any way.

I understand that's also a question of whether it will be properly maintained and kept up-to-date - all I can say here is that I'm pretty committed to that. Of course there's always possibility that it won't stay this way forever - if that ever happens, I see no obstacles on giving the project (and its npm package rights) away.

Let me know what you think.

*- to the degree allowed by restify itself - one recent example I can think of is its lack of support for mount paths.

tlivings commented 9 years ago

Sorry, I borked your merge due to some other merges.

tlivings commented 9 years ago

I wonder if there is something we can do to make swaggerize-express work with restify better so that there won't be a fork out there which is missing out on potentially important updates (like the recent security and vendor extensions update made in 4.0.0).

bardzusny commented 9 years ago

I fixed the mentioned merge conflict, everything should be OK now.

About making it possible to directly use swaggerize-express with restify - I'm really doubtful it would be viable, at all. Feel invited to look over my original commits to swaggerize-restify - I'm pretty sure making swaggerize-express take into account all the differences would lead to basically maintaining two sets of functions for much of its functionality - and tests as well, I believe.

I can't say I ever truly used or followed development of express that much - but I dare to say that, while originally incredibly similar in usage (from the REST use-case point of view), they diverged too much already for such approach to be sensible.