search

concentricsky / django-tastypie-swagger

An adapter to use swagger-ui with django-tastypie.
Other
132 stars 144 forks source link

upgrade swagger-ui to 1.2 #58

Closed Novarg closed 10 years ago

Novarg commented 10 years ago
johnraz commented 10 years ago

Hi, thanks for the contrib but 1.2 is not the latest version. We are actually aiming at v2.0.9 - please check #36 ;-)

fehguy commented 10 years ago

Hi folks, the current swagger specification is 1.2, the current swagger UI is 2.0.9. Many versions!

krimkus commented 10 years ago

Both libraries are v2.0+. Where are you seeing that the current swagger spec is at 1.2?

For reference: https://github.com/wordnik/swagger-js/tree/v2.0.23 https://github.com/wordnik/swagger-ui/tree/v2.0.12

fehguy commented 10 years ago

https://github.com/wordnik/swagger-core/wiki/1.2-transition

Note, libraries have version, but the swagger JSON has a structure, which is versioned as well!

krimkus commented 10 years ago

Ah, thanks for the clarification. I completely ignored where you said 'specification'. We should definitely check this pull request out and see if it can be resolved with PR #36.

johnraz commented 10 years ago

@krimkus : I already checked - it's just a bare replace of the js files and it is actually the wrong version ... I wouldn't have closed it otherwise.

krimkus commented 10 years ago

@johnraz Well there you go. I glanced at the updated swagger-ui.js, but it doesn't contain a version number so I couldn't tell.

johnraz commented 10 years ago

I'm being a total ass - it seems to be the right version (search the pr for swagger.js). Long day - sorry @fehguy : i'm still in doubt can you confirm how to check against both version please as nothing stands out in swagger-ui.js thx.

Novarg commented 10 years ago

Sorry i missed a version numbers. Actually it should be 2.0.23 and not 1.2 (this is specification version).

But currently i think that i made a bad thing with patching swagger.js here (i posted issue with patch https://github.com/wordnik/swagger-js/issues/84, this pull request already contain it). While it is working for my personal needs it is bad practice to provide that in this public repo (at least until swagger-ui maintainer accept or decline 84 issue)

fehguy commented 10 years ago

I'm looking into the swagger-js bug right now.

johnraz commented 10 years ago

@Novarg : any chance you could adapt this PR to use the non patched version of swagger.js ? I would really like to review / merge this in :)

Novarg commented 10 years ago

Yes i will try. Currently it won't work without that patch. So that's why i'd like to wait a decision on 84 issue first. If that's a bug i'd like to merge my patch in swagger.js and then it's ok to have it here. If i (and i suppose django-tastypie-swagger maintainer :) ) treat basePath wrongly and that patch will be declined i will revert it and will try to update django-tastypie-swagger itself to work correctly.

p.s. @krimkus @johnraz guys could you please share with me a reason (if you have any specific) why you have resources details under /doc/schema/<resource> endpoint rather then just /doc/<resource>? I do believe the latter one will fix the problem if 84 will be rejected. But seems i'm hurring again :)

numan commented 10 years ago

looks like https://github.com/wordnik/swagger-js/issues/84 might be fixed now.

Novarg commented 10 years ago

Yes, i noticed about that merge. I think i made this branch not really good to merge with additional commits that was necessary for my project because of lack of experience with github. I will figure this out when have a time.

johnraz commented 10 years ago

Guys, if you do work on this, keep in mind I started mapping 1.2 specs here.

I'm trying to do this at the same time I get tests ready so we don't break the library too much in the process ;-)

I don't have much time nowadays to push it forward so help is definitely welcome.

Seraf commented 10 years ago

Hello, is there some news about this ?

Novarg commented 10 years ago

well, my patch https://github.com/wordnik/swagger-js/issues/84 here was accepted so i suppose we are safe with this pull request (this copy of swagger.js already contain necessary patch).

johnraz commented 10 years ago

@Novarg is this still using the "hybrid" swagger file or the correct one from their repo ?

Novarg commented 10 years ago

It's hybrid but the same functionality was added to swagger so it doesn't really matter. I just checked swagger and your repo and my pull request looks already outdated so it's not worth to merge it at all.

I will open new pull request if i manage to update my fork to actual version again.

johnraz commented 10 years ago

Awesome @Novarg thx for you time again ;)