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

upgrade swagger-ui to 1.2 #58

Closed
wants to merge 3 commits into from
Closed

Conversation

Novarg
Copy link
Contributor

@Novarg Novarg commented Mar 5, 2014

  • Upgrade to the latest swagger-ui
  • support note attribute in custom endpoints

@johnraz
Copy link
Contributor

johnraz commented Mar 5, 2014

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

@johnraz johnraz closed this Mar 5, 2014
@fehguy
Copy link

fehguy commented Mar 5, 2014

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

@krimkus
Copy link
Contributor

krimkus commented Mar 5, 2014

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
Copy link

fehguy commented Mar 5, 2014

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
Copy link
Contributor

krimkus commented Mar 5, 2014

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.

@krimkus krimkus reopened this Mar 5, 2014
@johnraz
Copy link
Contributor

johnraz commented Mar 5, 2014

@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.

@johnraz johnraz closed this Mar 5, 2014
@krimkus
Copy link
Contributor

krimkus commented Mar 5, 2014

@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
Copy link
Contributor

johnraz commented Mar 5, 2014

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.

@johnraz johnraz reopened this Mar 5, 2014
@Novarg
Copy link
Contributor Author

Novarg commented Mar 5, 2014

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 swagger-api/swagger-js#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
Copy link

fehguy commented Mar 5, 2014

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

@johnraz
Copy link
Contributor

johnraz commented Mar 6, 2014

@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
Copy link
Contributor Author

Novarg commented Mar 6, 2014

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
Copy link

numan commented Jul 15, 2014

looks like swagger-api/swagger-js#84 might be fixed now.

@Novarg
Copy link
Contributor Author

Novarg commented Jul 16, 2014

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
Copy link
Contributor

johnraz commented Jul 16, 2014

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
Copy link

Seraf commented Sep 11, 2014

Hello, is there some news about this ?

@Novarg
Copy link
Contributor Author

Novarg commented Sep 11, 2014

well, my patch swagger-api/swagger-js#84 here was accepted so i suppose we are safe with this pull request (this copy of swagger.js already contain necessary patch).

@johnraz
Copy link
Contributor

johnraz commented Sep 11, 2014

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

@Novarg
Copy link
Contributor Author

Novarg commented Sep 11, 2014

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.

@Novarg Novarg closed this Sep 11, 2014
@johnraz
Copy link
Contributor

johnraz commented Sep 11, 2014

Awesome @Novarg thx for you time again ;)

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

Successfully merging this pull request may close these issues.

6 participants