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

Implements #10 - validator.find() #37

Merged
merged 5 commits into from
Jan 27, 2020
Merged

Implements #10 - validator.find() #37

merged 5 commits into from
Jan 27, 2020

Conversation

codeclown
Copy link
Contributor

@codeclown codeclown commented Oct 21, 2018

Addresses issue #10.

This is from an older branch I made when I needed this functionality in a project.

Let me know if there's something to adjust (namings, tests, something else) and I'll happily revisit this.

@Hilzu
Copy link
Owner

Hilzu commented Oct 23, 2018

Just came back from holiday and will look at this PR this week. Before that can you rebase this on master, fix the tests and add tests to keep code coverage at 100%? The travis build and coveralls should be working now.

@backbone87
Copy link

path-to-regexp uses express style path, while OAS uses another syntax for path parameter, this implementation does not work with OAS path's like /user/{id}

@codeclown
Copy link
Contributor Author

codeclown commented Oct 16, 2019

Finally taking a stab at resolving this PR.

@backbone87 Thanks for pointing that out!

I checked the V3 spec about paths and it seems like OpenAPI supports path params in format {name} and nothing else (also not allowing slashes), so I've implemented a simple transform from OAS to Express path before passing onto path-to-regexp (could've removed the dependency altogether and implemented the regex logic in this project, but I felt it would've been unnecessary).


I have a problem making the OpenApiValidator.test.ts test case pass because TypeScript will not accept the second parameter. Types come from here. Talking about this line in the test:

match(req, { ...baseRes }, () => {});

I've tried null (obvious fail) and new express.Response (not so obvious fail).

And the build is failing due to something unrelated:

[error] "2019-10-16T10:11:50.900Z"  'error from lcovParse: ' 'Failed to parse string'
[error] "2019-10-16T10:11:50.903Z"  'input: ' ''
[error] "2019-10-16T10:11:50.903Z"  'error from convertLcovToCoveralls'

@coveralls
Copy link

coveralls commented Jan 21, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling b1c5a45 on codeclown:match into c647e29 on Hilzu:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a3d07cd on codeclown:match into c647e29 on Hilzu:master.

@codeclown
Copy link
Contributor Author

@Hilzu Tests are green.

@Hilzu Hilzu merged commit 629e139 into Hilzu:master Jan 27, 2020
@Hilzu
Copy link
Owner

Hilzu commented Jan 27, 2020

This looks good. Thank you for your contribution and thanks for waiting this long to get the change merged.

@codeclown
Copy link
Contributor Author

@Hilzu Thanks for merging. Added some docs as well in separate PR: #37

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.

4 participants