-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: support server variables #222
Conversation
…esponse Validator
…lidators into 89-add-server-variable-support
Codecov Report
@@ Coverage Diff @@
## master #222 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 17 +1
Lines 373 400 +27
Branches 49 56 +7
=========================================
+ Hits 373 400 +27
Continue to review full report at Codecov.
|
So I wasn't aware that Travis recently changed their pricing plan such that open source projects no longer have a generous allowance of builds. We seem to have very quickly used up our one-off allowance. Following their FAQ, I've requested more build minutes - hopefully they'll be generous. If not, we'll have to migrate to another build provider, perhaps GitHub Actions, though I haven't looked into it yet |
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 good for merging and publishing after CI is fixed and I've corrected the two test names
packages/jest-openapi/__test__/matchers/toSatisfyApiSpec/serversDefinedDifferently.test.js
Outdated
Show resolved
Hide resolved
packages/jest-openapi/__test__/matchers/toSatisfyApiSpec/serversDefinedDifferently.test.js
Outdated
Show resolved
Hide resolved
Object.entries(serverVariables).reduce( | ||
(currentMap, [variableName, detailsOfPossibleValues]) => ({ | ||
...currentMap, | ||
[variableName]: getPossibleValuesOfServerVariable( |
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.
note: we use variableName
here but variable
on line 39. I think by definition a variable is a variableName, but in the openapi spec a server variable
is a complex object. This is an example of how migrating our code to TypeScript in future will hopefully help clarify it
@@ -0,0 +1,91 @@ | |||
const generateCombinations = require('combos'); |
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 file could definitely be simpler, but I'm not sure how. Will be very grateful for suggestions! The tricky thing is to provide error messages with server urls that are templated before the path, and concrete after the path, e.g. https://{hostVariable}.com:{portVariable}/defaultValueOfVariableInPath
(See the tests)
Migrating away from Travis CI because we [no longer get free builds :(](#222 (comment))
For #89
Supercedes #161 and #218. Thanks @JonnySpruce for your work on this!
I think I've overcomplicated your code in order to output these error messages. Perhaps we can simplify it in future :)