Use definitionsFullyResolved to look up consumes/produces + tests #117
Conversation
…ies - fixes issue when using split definition files
lumaxis
force-pushed
the
definitionsFullyResolved
branch
from
1904c77 to
8b6f9be
Compare
lumaxis
force-pushed
the
definitionsFullyResolved
branch
from
8b6f9be to
6713ef2
Compare
| @@ -64,6 +66,24 @@ module.exports.getSwaggerApi = function (callback) { | |||
| } | |||
| }; | |||
|
|
|||
| module.exports.getSwaggerApiRelativeRefs = function (callback) { | |||
There was a problem hiding this comment.
Where is this used? If it's unused, it should be removed from the PR.
There was a problem hiding this comment.
There was a problem hiding this comment.
I guess I hadn't scrolled down enough. I'll finish the review.
There was a problem hiding this comment.
Yea, GitHub hides the diff of that file by default, you have to explicitly load it 🙂
Also, just wanna let you know, that diff of that file looks way more complicated on GitHub than it actually is. It's really just copying all of the tests, indenting them one more level and changing them to use swagger-relative-refs.
| @@ -32,260 +32,638 @@ var helpers = require('./helpers'); | |||
| var Sway = helpers.getSway(); | |||
|
|
|||
| describe('Operation', function () { | |||
| var swaggerApi; | |||
| context('swaggerApiRelativeRefs', function () { | |||
There was a problem hiding this comment.
Any reason this is necessary? In previous tests where we use ./2.0/swagger-relative-refs.yaml we just do it in that specific test. Creating a SwaggerApi object using Sway#create with that file vs. using a helper is trivial so do we really need it?
(If we decide "No", we could remove the helper completely.)
There was a problem hiding this comment.
It's not strictly necessary but I added it because I had to supply jsonRefs manually to get the local refs parsed correctly.
Also, looking at the test you linked, I think this test is actually nor working. The result of Swagger.create appears to not be used at all and the test is really testing the object from line 39.
To sum it up, I'm happy to remove the helper but after considering the above, I'd suggest to use the helper in test-issues.js too.
There was a problem hiding this comment.
Yeah, let's move it to test-issues.js and I'll merge it. Having a helper makes sense. Also, this is the only use of context as well so maybe we can do a per-test invocation of your helper?
There was a problem hiding this comment.
@whitlockjc Okay, I fixed the test in test-issues.js but I'm not sure I understand your question about context. context is just an alias of describe and I used it to group the two test groups for the "regular" swaggerApi and one using relative refs. Could you clarify what you meant?
There was a problem hiding this comment.
I just meant that for consistency, it might be best to use describe instead of context.
Also, what editor are you using?
There was a problem hiding this comment.
That is VS Code with an Atom theme 🙂
Sure, I can switch back to describe 👍 I just used it as I prefer context in my personal code because it reads a bit nicer.
There was a problem hiding this comment.
I'm using the same setup, VS Code and the Atom One Dark theme. As for context vs. describe, I think it's out of habit honestly. But in this case, consistency is more important.
lumaxis
force-pushed
the
definitionsFullyResolved
branch
from
6713ef2 to
fdac8fc
Compare
lumaxis
force-pushed
the
definitionsFullyResolved
branch
from
fdac8fc to
6d6437c
Compare
|
Merged. |
@whitlockjc This includes the changes from #101 plus I added tests.
For the tests, I basically duplicated the existing tests for
operation.jsbut they are now run on bothswagger.yamlandswagger-relative-refs.yaml.Let me know if you'd prefer a different approach 👍
Closes #101, fixes #92