-
Notifications
You must be signed in to change notification settings - Fork 99
GoDaddy Contributions #52
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
Changes from all commits
88c330a
a9c1152
5a07b80
59afbb1
0f4bf48
ed04446
0e22384
15aa4e9
17e8b7b
c1a0eb5
2c737d4
3dc7de6
28d293b
86d7958
696e387
24d77f0
ae8858c
9693150
1a96d84
c6754ba
cf3e616
1eddcad
5ef5bae
820c51f
76057a2
a4ab802
d7909e8
464e37c
240898e
2d6f7ae
8563dd0
7f4e8b3
fd30b2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,12 @@ lib-cov | |
| *.pid | ||
| *.gz | ||
|
|
||
| .idea | ||
| pids | ||
| logs | ||
| results | ||
|
|
||
| npm-debug.log | ||
| node_modules | ||
|
|
||
| LoopBackExplorer.iml | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ var classHelper = module.exports = { | |
| */ | ||
| generateAPIDoc: function(aClass, opts) { | ||
| return { | ||
| apiVersion: opts.version, | ||
| apiVersion: opts.version || '1', | ||
| swaggerVersion: opts.swaggerVersion, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we are at fixing version numbers, I am proposing to fill @STRML @raymondfeng any objections?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds good. Any other value than 1.2 would be wrong here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current master was already providing a hard-coded version string |
||
| basePath: opts.basePath, | ||
| resourcePath: urlJoin('/', opts.resourcePath), | ||
|
|
@@ -43,7 +43,7 @@ var classHelper = module.exports = { | |
| generateResourceDocAPIEntry: function(aClass) { | ||
| return { | ||
| path: aClass.http.path, | ||
| description: aClass.ctor.sharedCtor && aClass.ctor.sharedCtor.description | ||
| description: aClass.ctor.settings.description || aClass.ctor.sharedCtor && aClass.ctor.sharedCtor.description | ||
| }; | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,18 @@ var modelHelper = module.exports = { | |
| * @return {Object} Associated model definition. | ||
| */ | ||
| generateModelDefinition: function generateModelDefinition(modelClass, definitions) { | ||
| var processType = function(app, modelName, referencedModels) { | ||
| if (app && modelName) { | ||
| if (modelName.indexOf('[') == 0) { | ||
| modelName = modelName.replace(/[\[\]]/g, ''); | ||
| } | ||
| var model = app.models[modelName]; | ||
| if (model && referencedModels.indexOf(model) === -1) { | ||
| referencedModels.push(model); | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shelbys this looks suspicious to me. Can you describe a scenario that needs this change? AFAIK, /cc @raymondfeng thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferring If we want to support rawProperties, then we have to duplicate the logic from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Github is displaying an incorrect diff for me. Here is the target of my comments above: - var properties = _cloneDeep(def.properties);
+ var properties = _cloneDeep(def.rawProperties || def.properties);
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am rejecting this part of the PR. @shelbys if you can explain why this change is necessary, I am happy to reconsider.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - I switched to rawProperties in order to have access to the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the current master already handles nested model references - see lib/model-helper.js#L73-L86.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - Please confirm that master handles all of: 1) recursively nested references to 2) Models and 3) Arrays of Models in 4) properties, 5) modelTo and modelThrough relations, 6) accepts, 7) returns and 8) errors
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See #61. |
||
| } | ||
|
|
||
| var def = modelClass.definition; | ||
| var name = def.name; | ||
| var out = definitions || {}; | ||
|
|
@@ -27,15 +39,15 @@ var modelHelper = module.exports = { | |
| } | ||
| var required = []; | ||
| // Don't modify original properties. | ||
| var properties = _cloneDeep(def.properties); | ||
| var properties = _cloneDeep(def.rawProperties || def.properties); | ||
|
|
||
| var referencedModels = []; | ||
| // Add models from settings | ||
| if (def.settings && def.settings.models) { | ||
| for (var m in def.settings.models) { | ||
| var model = modelClass[m]; | ||
| if (typeof model === 'function' && model.modelName) { | ||
| if (referencedModels.indexOf(model) === -1) { | ||
| if (model && referencedModels.indexOf(model) === -1) { | ||
| referencedModels.push(model); | ||
| } | ||
| } | ||
|
|
@@ -58,6 +70,10 @@ var modelHelper = module.exports = { | |
|
|
||
| // Eke a type out of the constructors we were passed. | ||
| prop = modelHelper.LDLPropToSwaggerDataType(prop); | ||
| processType(modelClass.app, prop.type, referencedModels); | ||
| if (prop.items) { | ||
| processType(modelClass.app, prop.items.type, referencedModels); | ||
| } | ||
|
|
||
| // Required props sit in a per-model array. | ||
| if (prop.required || (prop.id && !prop.generated)) { | ||
|
|
@@ -95,15 +111,43 @@ var modelHelper = module.exports = { | |
| // Generate model definitions for related models | ||
| for (var r in modelClass.relations) { | ||
| var rel = modelClass.relations[r]; | ||
| if (rel.modelTo){ | ||
| generateModelDefinition(rel.modelTo, out); | ||
| if (rel.modelTo && referencedModels.indexOf(rel.modelTo) === -1) { | ||
| referencedModels.push(rel.modelTo); | ||
| } | ||
| if (rel.modelThrough && referencedModels.indexOf(rel.modelThrough) === -1) { | ||
| referencedModels.push(rel.modelThrough); | ||
| } | ||
| if (rel.modelThrough) { | ||
| generateModelDefinition(rel.modelThrough, out); | ||
| } | ||
|
|
||
| if (modelClass.sharedClass) { | ||
| var remotes = modelClass.sharedClass.methods(); | ||
| for (var remoteIdx in remotes) { | ||
| var remote = remotes[remoteIdx]; | ||
| var accepts = remote.accepts; | ||
| if (accepts) { | ||
| for (var acceptIdx in accepts) { | ||
| processType(modelClass.app, accepts[acceptIdx].type, referencedModels); | ||
| } | ||
| } | ||
| var returns = remote.returns; | ||
| if (returns) { | ||
| for (var returnIdx in returns) { | ||
| processType(modelClass.app, returns[returnIdx].type, referencedModels); | ||
| } | ||
| } | ||
| var errors = remote.errors; | ||
| if (errors) { | ||
| for (var errorIdx in errors) { | ||
| processType(modelClass.app, errors[errorIdx].responseModel, referencedModels); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (var i = 0, n = referencedModels.length; i < n; i++) { | ||
| generateModelDefinition(referencedModels[i], out); | ||
| if (referencedModels[i].definition) { | ||
| generateModelDefinition(referencedModels[i], out); | ||
| } | ||
| } | ||
| return out; | ||
| }, | ||
|
|
@@ -113,7 +157,7 @@ var modelHelper = module.exports = { | |
| * get a string type. | ||
| * @param {*} propType Prop type description. | ||
| * @return {String} Prop type string. | ||
| */ | ||
| */ | ||
| getPropType: function getPropType(propType) { | ||
| if (typeof propType === 'function') { | ||
| // See https://github.com/strongloop/loopback-explorer/issues/32 | ||
|
|
@@ -126,7 +170,7 @@ var modelHelper = module.exports = { | |
| }, | ||
|
|
||
| isHiddenProperty: function(definition, propName) { | ||
| return definition.settings && | ||
| return definition.settings && | ||
| Array.isArray(definition.settings.hidden) && | ||
| definition.settings.hidden.indexOf(propName) !== -1; | ||
| }, | ||
|
|
@@ -135,6 +179,11 @@ var modelHelper = module.exports = { | |
| // Swagger spec. | ||
| // https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#431-primitives | ||
| LDLPropToSwaggerDataType: function LDLPropToSwaggerDataType(prop) { | ||
| if (typeof prop === 'string') { | ||
| prop = { | ||
| type: prop | ||
| } | ||
| } | ||
| var out = _cloneDeep(prop); | ||
| out.type = modelHelper.getPropType(out.type); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,12 +113,71 @@ var routeHelper = module.exports = { | |
| * See https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#523-operation-object | ||
| */ | ||
| routeToAPIDoc: function routeToAPIDoc(route, classDef) { | ||
| /** | ||
| * Converts from an sl-remoting data type to a Swagger dataType. | ||
| */ | ||
| function prepareDataType(type) { | ||
| if (!type) { | ||
| return 'void'; | ||
| } | ||
|
|
||
| if(Array.isArray(type)) { | ||
| if (type.length > 0) { | ||
| if (typeof type[0] === 'string') { | ||
| return '[' + type[0] + ']'; | ||
| } else if (typeof type[0] === 'function') { | ||
| return '[' + type[0].name + ']'; | ||
| } else if (typeof type[0] === 'object') { | ||
| if (typeof type[0].type === 'function') { | ||
| return '[' + type[0].type.name + ']'; | ||
| } else { | ||
| return '[' + type[0].type + ']'; | ||
| } | ||
| } else { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shelbys You are removing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems this is actually correct, the spec for Operation Object does not mention any
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, it does not mention the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swagger provides two ways for specifying the response type:
In the screenshot above, I'll submit a pull request that keeps the response model in both places. We can discuss what's the best solution in the PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR: #61 Please post any replies there.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - We switched to always using responseMessage.responseModel, because we use different Models by Status, including a custom, reusable Model for Error responses. This is more natural for many of our endpoints that aren't CRUD-related. Also, I believe it better uses the space visually and with less confusion. You could change it to use For reference, see: https://developer.godaddy.com/doc#!/shoppers/Subaccount_create |
||
| return '[' + type + ']'; | ||
| } | ||
| } | ||
| return 'array'; | ||
| } | ||
|
|
||
| // TODO(schoon) - Add support for complex dataTypes, "models", etc. | ||
| switch (type) { | ||
| case 'Array': | ||
| return 'array'; | ||
| case 'Boolean': | ||
| return 'boolean'; | ||
| case 'buffer': | ||
| return 'string'; | ||
| case 'Date': | ||
| return 'date'; | ||
| case 'number': | ||
| case 'Number': | ||
| return 'double'; | ||
| case 'Object': | ||
| return 'object'; | ||
| case 'String': | ||
| return 'string'; | ||
| } | ||
|
|
||
| return type; | ||
| } | ||
|
|
||
| var returnDesc; | ||
|
|
||
| // Some parameters need to be altered; eventually most of this should | ||
| // be removed. | ||
| var accepts = routeHelper.convertAcceptsToSwagger(route, classDef); | ||
| var returns = routeHelper.convertReturnsToSwagger(route, classDef); | ||
| var responseMessages = [ | ||
| { | ||
| code: 200, | ||
| message: 'Request was successful', | ||
| responseModel: returns.model || prepareDataType(returns.type) || 'void' | ||
| } | ||
| ]; | ||
| if (route.errors) { | ||
| responseMessages.push.apply(responseMessages, route.errors); | ||
| } | ||
|
|
||
| debug('route %j', route); | ||
|
|
||
|
|
@@ -128,17 +187,15 @@ var routeHelper = module.exports = { | |
| // `items` and `format` fields. | ||
| operations: [routeHelper.extendWithType({ | ||
| method: routeHelper.convertVerb(route.verb), | ||
| deprecated: route.deprecated, | ||
| // [rfeng] Swagger UI doesn't escape '.' for jQuery selector | ||
| nickname: route.method.replace(/\./g, '_'), | ||
| // Per the spec: | ||
| // https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#523-operation-object | ||
| // This is the only object that may have a type of 'void'. | ||
| type: returns.model || returns.type || 'void', | ||
| parameters: accepts, | ||
| // TODO(schoon) - We don't have descriptions for this yet. | ||
| responseMessages: [], | ||
| summary: route.description, // TODO(schoon) - Excerpt? | ||
| notes: '' // TODO(schoon) - `description` metadata? | ||
| notes: route.notes, // TODO(schoon) - `description` metadata? | ||
| consumes: ['application/json', 'application/xml', 'text/xml'], | ||
| produces: ['application/json', 'application/javascript', 'application/xml', 'text/javascript', 'text/xml'], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the values for The produced content types depends on the version of strong-remoting and can be extended by remote methods, as they may decide to use a custom content type. For the sake of simplicity, I am ok with ignoring this complexity and reporting only the base types supported by strong-remoting OOTB. @raymondfeng @ritch @STRML thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shelbys are you including 'application/javascript', 'text/javascript' to support JSONP requests? (Best Content-Type to serve JSONP)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider this comment as resolved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - Yes, I added those to support JSONP, and I only put them on produces, because I didn't have a reason to accept JSONP vs plain JSON |
||
| parameters: accepts, | ||
| responseMessages: responseMessages | ||
| })] | ||
| }; | ||
|
|
||
|
|
@@ -192,15 +249,21 @@ var routeHelper = module.exports = { | |
| } | ||
|
|
||
| var out = { | ||
| paramType: paramType || type, | ||
| name: name, | ||
| description: accepts.description, | ||
| type: accepts.type, | ||
| required: !!accepts.required, | ||
| paramType: paramType || type, | ||
| type: accepts.type, | ||
| $ref: accepts.model, | ||
| items: accepts.items, | ||
| uniqueItems: accepts.uniqueItems, | ||
| format: accepts.format, | ||
| pattern: accepts.pattern, | ||
| defaultValue: accepts.defaultValue, | ||
| enum: accepts.enum, | ||
| minimum: accepts.minimum, | ||
| maximum: accepts.maximum, | ||
| allowMultiple: false | ||
| allowMultiple: accepts.allowMultiple, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @STRML @raymondfeng I need your help here as my knowledge of Swagger is not good enough. Metadata like How about the new metadata - The following metadata is not inferred from the type (yet) and thus it's IMO ok to customise them: However, it leads me to the conclusion, that values specified in remoting metadata should be treated as a default and they should be overridden by I have no idea how to treat
It seems to me that this flag depends very much on what strong-remoting is willing to accept and process. If we support comma-separated values and parse them into an array, then this should be always true for relevant param types. If we don't support it, then this should be always false. Unless I am interpreting this in a completely wrong way. Thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raymondfeng @STRML ping. This is the last part of this PR that needs to be ported.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos I agree with you on how we should deal with Swagger uses JSON schema for typing properties. There are three styles:
$ref is a JSON schema construct to reference a JSON model by pointer, for example, "$ref": "#/models/MyModel". I'm not sure if accepts.model is in that format. My take is to use LDL type information and translate them into JSON schema.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in #64. The following fields are copied from LDL to Swagger: 'format',
'defaultValue', // "default" in LDL
'enum',
'minimum', // "min" in LDL
'maximum', // "max" in LDL
'uniqueItems',
// loopback-explorer extensions
'length',
// https://www.npmjs.org/package/swagger-validation
'pattern'
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - Are you guys using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK we don't use either. I was trying to find out what part of Swagger is using the field
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - I took |
||
| description: accepts.description | ||
| }; | ||
|
|
||
| out = routeHelper.extendWithType(out); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,8 @@ function Swagger(loopbackApplication, swaggerApp, opts) { | |
| basePath: loopbackApplication.get('restApiRoot') || '/api', | ||
| resourcePath: 'resources', | ||
| // Default consumes/produces | ||
| consumes: ['application/json', 'application/x-www-form-urlencoded'], | ||
| produces: ['application/json'], | ||
| consumes: ['application/json', 'application/x-www-form-urlencoded', 'application/xml', 'text/xml'], | ||
| produces: ['application/json', 'application/javascript', 'application/xml', 'text/javascript', 'text/xml'], | ||
| version: getVersion() | ||
| }); | ||
|
|
||
|
|
@@ -100,9 +100,9 @@ function addRoute(app, uri, doc, opts) { | |
| // know that header at the time the data is built. | ||
| if (hasBasePath) { | ||
| var headers = req.headers; | ||
| var host = headers.Host || headers.host; | ||
| doc.basePath = (opts.protocol || req.protocol) + '://' + | ||
| host + initialPath; | ||
| var host = headers['x-forwarded-host'] || headers['X-Forwarded-Host'] || headers.Host || headers.host; | ||
| var protocol = headers['x-forwarded-proto'] || headers['X-Forwarded-Proto'] || opts.protocol || req.protocol | ||
| doc.basePath = protocol + '://' + host + initialPath; | ||
| } | ||
| res.status(200).send(doc); | ||
| }); | ||
|
|
@@ -118,13 +118,14 @@ function generateResourceDoc(opts) { | |
| return { | ||
| swaggerVersion: opts.swaggerVersion, | ||
| apiVersion: opts.version, | ||
| apis: [], | ||
| // See https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#513-info-object | ||
| info: opts.apiInfo | ||
| info: opts.apiInfo, | ||
| // TODO Authorizations | ||
| // https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#514-authorizations-object | ||
| // TODO Produces/Consumes | ||
| // https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#52-api-declaration | ||
| consumes: ['application/json', 'application/xml', 'text/xml'], | ||
| produces: ['application/json', 'application/javascript', 'application/xml', 'text/javascript', 'text/xml'], | ||
| apis: [], | ||
| models: opts.models | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shelbys According to Swagger Spec, the resource-listing object does not have any of the properties
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am rejecting this part of the PR. @shelbys if you can explain why this change is necessary, I am happy to reconsider.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos I agree with you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - I ported all of those from However, it seemed nice to set a default for consumes/produces, and then only have to override when different, especially since strong-remoting provides automatic conversion now. Also, we definitely have Models (Error, ErrorField) that would be nice to specify once, and then reuse across multiple resources, but Swagger wants Resources to be self-contained instead |
||
| }; | ||
| } | ||
|
|
||
|
|
@@ -137,7 +138,7 @@ function getVersion() { | |
| try { | ||
| version = require(path.join(process.cwd(), 'package.json')).version; | ||
| } catch(e) { | ||
| version = ''; | ||
| version = '1'; | ||
| } | ||
| return version; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| "url": "https://github.com/strongloop/loopback-explorer/issues" | ||
| }, | ||
| "devDependencies": { | ||
| "loopback": "1.x", | ||
| "loopback": "git+https://github.com/shelbys/loopback.git", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This MUST be fixed before landing the patch.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raymondfeng - NOTE, we only forked loopback to easily reference our forks of the submodules (e.g. loopback-boot, loopback-explorer, strong-remoting, etc), so this change can be reverted before merge |
||
| "mocha": "~1.20.1", | ||
| "supertest": "~0.13.0", | ||
| "chai": "~1.9.1" | ||
|
|
@@ -31,7 +31,7 @@ | |
| "url": "https://github.com/strongloop/loopback-explorer/blob/master/LICENSE" | ||
| }, | ||
| "dependencies": { | ||
| "swagger-ui": "~2.0.18", | ||
| "swagger-ui": "git+https://github.com/shelbys/swagger-ui.git", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This MUST be fixed before landing the patch.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raymondfeng - I'm not sure how to handle this one, because https://github.com/shelbys/swagger-ui will need to be merged into https://github.com/wordnik/swagger-ui and https://github.com/shelbys/swagger-js into https://github.com/wordnik/swagger-js in order to avoid a dependency on my fork. The functional changes in that fork should be universal, however there are some GoDaddy styling changes that have crept into the LESS/CSS, which will need to be done another way
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shelbys thanks for clarifying this, I presume it is up to us to submit the changes from your forks to the upstream repos and do any changes necessary to get the patches landed. I am assuming the changes in swagger-ui and swagger-js are not tied to this pull request and thus can be upstreamed later, after this PR is resolved. Is that correct? /cc @altsang @altsang I did not anticipate this task when estimating the effort, almost certainly I won't be able to prepare swagger-ui and swagger-js changes this sprint.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify.
My assumption is that if we land the changes in this PR and keep using the unmodified swagger-ui and swagger-js, things will continue to work. Therefore we can land this PR without having to wait until swagger-ui/js is updated. @shelbys Is that correct?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bajtos - We'll need to coordinate on the swagger-js and swagger-ui PRs, because you have style overrides and we have overrides, so we need to find a way for those to work together. We need a way to easily have local colors and fonts, but we should be able to agree on layout and spacing. If we can find a way to make the top-level and embedded cases both work and still have custom styling, I'm happy to strip out the styling changes from swagger-ui and then submit the rest. I believe swagger-js only has universal changes, but includes an extension to Swagger Spec for documenting multiple basePaths for multiple environments. I'm not sure how they'll feel about the last, but am happy to keep it in my fork if necessary The only functionality loss I see for sure is the Response Content Type not being shown for Response Messages, see: shelbys/swagger-ui@16ad569 Everything else seems to be bug fixes and usability
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this discussion to #66. |
||
| "debug": "~1.0.3", | ||
| "lodash.clonedeep": "^2.4.1", | ||
| "lodash.defaults": "^2.4.1", | ||
|
|
||

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.
@shelbys what is the rationale for this, why don't you fill the version number in explorer options?
I would prefer to use
1.0as the default value, as it's closer to http://semver.org/. Any objections?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.
We are actually using the version number from
package.jsonor1.0.0now.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.
@bajtos - I believe I was running into something referencing the value without checking for its presence. This may not be necessary any longer. I used '1' instead of '1.0', because path-based versioning is generally done using integer versions only