-
Notifications
You must be signed in to change notification settings - Fork 54
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
Improve casing errors #202
Conversation
vladbarosan
commented
Jan 31, 2018
•
edited
Loading
edited
- Fix all linter issues
- add option for case insensitive live validation path matching
Waiting on the sway PR to get merged https://github.com/amarzavery/sway/pull/20 |
@@ -92,6 +92,7 @@ class SpecValidator { | |||
options.definition = self.specInJson; | |||
options.jsonRefs = {}; | |||
options.jsonRefs.relativeBase = self.specDir; | |||
options.isPathCaseSensitive = self.options.isPathCaseSensitive; |
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.
self.options.isPathCaseSensitive;
Please document this option in the constructor documentation of SpecValidator class
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.
Done
25d9c63
to
aaa3851
Compare
'EQUIVALENT_PATH': 'M6008', | ||
'DUPLICATE_PARAMETER': 'M6009', |
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.
Why did we remove these constants from the mapper?
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.
@amarzavery because those 2 are duplicates. Not sure if the mapped values are used anywhere with a hard dependency so I didn't adapt the numbers for remaining ones. Let me know if I should
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.
@vishrutshah - There were some duplicates here. Do you know what is the effect of these error codes in reporting?
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.
@amarzavery I don't thing right now they are used for anything since we don't have official documentation from them also I don't thing @vishrutshah spends his time on Github anymore :)
package.json
Outdated
"jshint": "jshint index.js --reporter=jslint", | ||
"test": "npm -s run-script jshint && mocha -t 100000", | ||
"jshint": "jshint lib --reporter=jslint", | ||
"test": "mocha -t 100000", |
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.
does npm run jshint by itself, now? Previously it did not do that. Hence we had to && the jshint script along with mocha.
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.
Yup it does not run jshint by itself. https://travis-ci.org/Azure/oav/builds/335452396#L970
So please do the following:
"test": "npm -s run-script jshint && mocha -t 100000",
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.
@amarzavery left it out because there as still a couple of linter issues which I was not sure to resolve. Let me look if I can get it clean and then I can add it back
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.
Ah I see. I thought you fixed all of them. No worries, if you cannot fix all of them right now.
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.
@amarzavery Getting there lets clean it up and keep it like that :)
any idea why we iterate here, doesn't seem we do anything different in each iteration:
oav/lib/xMsExampleExtractor.js
Line 192 in 4459e37
for (var response in responses) { |
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.
you wont be able to replace this with a for..of
. We are iterating over the responses dictionary in swagger for a given operation to extract examples for every response code
"responses": {
"200": {},
"202": {},
. . .
}
please update the version in package.json and also update the changelog. |