-
Notifications
You must be signed in to change notification settings - Fork 80
Fix #234, proper error handling #251
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
Conversation
|
I have the same fix in #248. Guess there are still some promise issues in |
|
@MikeRalphson I know there have been a lot of changes in oas-validator recently, should I merge this or wait for the updates? |
|
@pderaaij hey could you update the changelog for this? It should go under an |
|
@philsturgeon this appears not to be the correct fix. I found the correct one for #248. I will update the PR in a few minutes. |
|
@philsturgeon Updated the PR, this is a proper fix |
|
Thank you @pderaaij. I'm going to leave it to @MikeRalphson to review this one, if he approves I'll be happy to merge it. |
|
@pderaaij should I test this one against the released |
|
@MikeRalphson I've tested it against The fix should work in both cases. |
MikeRalphson
left a comment
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.
Should it not be?
const { context, lintResults, valid } = _options || err.options;
if (err && valid === false) {
|
With the above change, |
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.
Now LGTM! PR should probably be squashed to aid any future git bisect etc.
|
Thanks for the review. I need to get used to all the new options in Javascript. Long time ago I developed in javascript/node.js ;-) |
Doesn't show, I'd hate to try and write any Java or C# today! |
|
Is there a way for me to fetch a Docker image with this baked in so I can test it against #253? |
|
@dkarlovi sorry bud I was asleep! :) Merged now, so docker hub should be updated soon. |
|
@philsturgeon sweet, just ran nightly, it indeed fixed the issue, thanks. 👍 |
No description provided.