Skip to content
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

fix: validating output status code #648

Merged
merged 11 commits into from
Sep 26, 2019

Conversation

karol-maciaszek
Copy link
Contributor

@karol-maciaszek karol-maciaszek commented Sep 25, 2019

Closes #647

Checklist

  • Tests have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Bugfix

What is the current behavior? What is the new behavior?

Response's status code is not validated against responses found in the spec.

Does this PR introduce a breaking change?

No (we do not have proxy functionality yet, right?)

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Some questions, a proposal and a doubt, but we're almost there.

packages/http/src/validator/utils/spec.ts Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
packages/http/src/validator/utils/spec.ts Show resolved Hide resolved
packages/http/src/validator/utils/spec.ts Show resolved Hide resolved
packages/http/src/validator/__tests__/index.spec.ts Outdated Show resolved Hide resolved
packages/http/src/validator/utils/__tests__/spec.spec.ts Outdated Show resolved Hide resolved
packages/http/src/validator/utils/__tests__/spec.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is good. I'm not approving because you said you want to try to convert this into an Option — let me know if you need help or you want to defer the task and I'll approve right away.

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put some comments and suggestions on how to make this a little bit easier to reason about but good job!

packages/http/src/validator/index.ts Outdated Show resolved Hide resolved
packages/http/src/validator/index.ts Outdated Show resolved Hide resolved
packages/http/src/validator/index.ts Outdated Show resolved Hide resolved
packages/http/src/validator/utils/__tests__/spec.spec.ts Outdated Show resolved Hide resolved
packages/http/src/validator/utils/spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pre-approving. We need @philsturgeon to answer our question and then I'll leave the merge to you so you can check out the changes I did to make TypeScript happy again.

@karol-maciaszek karol-maciaszek merged commit fe7d4cc into master Sep 26, 2019
@karol-maciaszek karol-maciaszek deleted the fix/validating-output-status-code branch September 26, 2019 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output validator is not checking the returned status code
3 participants