-
Notifications
You must be signed in to change notification settings - Fork 24
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
Make the API playground work against Swagger 2.0 implementation in Gravity. #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my review, with the caveat that I've never written coffee script, used Swagger or dealt with front-end code in a react context. :)
@@ -1,16 +1,32 @@ | |||
# This configuration was generated by | |||
# `rubocop --auto-gen-config` | |||
# on 2017-10-11 13:16:21 -0400 using RuboCop version 0.50.0. | |||
# on 2018-10-25 13:54:35 -0400 using RuboCop version 0.59.2. | |||
# The point is for the user to remove these configuration records | |||
# one by one as the offenses are removed from the code base. | |||
# Note that changes in the inspected code, or installation of new | |||
# versions of RuboCop, may require this file to be generated again. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to call out that this file looks like it's meant to get smaller but seems in fact to be getting bigger. If we care about rubocop linting ought we consider taking a moment to clean this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but in a separate PR. My workflow for this is rubocop -a ; rubocop --auto-gen-config
.
@@ -1,5 +1,5 @@ | |||
/* | |||
*= require swagger-lib | |||
*= require swagger-ui-dist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, by using swagger-ui-dist
rather than just swagger-ui-lib
you're pulling a pre-built set of swagger files from NPM, rather than pulling it in as a managed dependency, right?
I've not yet done any kind of front-end work within a Ruby on Rails context, is this standard? Do we generally want to avoid NPM? Is there some alternative approach possible where we add swagger-ui-lib
to a package.json
file and let NPM take care of it, rather than just dumping a prebuilt version into the assets folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old way was cherry picking files from source. Since then swagger-ui released a self-contained distribution without dependencies, and is now the recommended way of using that in Rails. Npm is just a download mechanism here, I just couldn't find a tarball or something like that to get.
Generally we don't add node package managers into Rails apps :)
.swagger-section | ||
#swagger-ui-container.swagger-ui-wrap | ||
#swagger-ui |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 2.0 is a bit cleaner!
@@ -0,0 +1,14 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lib seems to go to a lot of trouble to expose this getAbsoluteFSPath
method but we're not calling it anywhere. I assume that that's just because we don't need it, because its static files are all in a place where Rails knows how to serve them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was for backwards compatibility with some really old swagger implementations. We haven't used this ever and don't care.
LMK if there's anything you want me to do for this PR to get merged @mbilokonsky |
Followup from https://github.com/artsy/gravity/pull/11928, which broke https://developers.artsy.net/playground