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

Add documentation for match() #63

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Add documentation for match() #63

merged 2 commits into from
Jan 31, 2020

Conversation

codeclown
Copy link
Contributor

@codeclown codeclown commented Jan 28, 2020

Documentation proposal for feature from #37


Maintainers, feel free to edit any formatting or words as you wish.

@coveralls
Copy link

coveralls commented Jan 28, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 45a7b0d on codeclown:match-docs into e0d7377 on Hilzu:master.

@Hilzu
Copy link
Owner

Hilzu commented Jan 28, 2020

You should document what the pitfall of using match compared to validate is. With validate you get an error if you try to add validation for an operation that doesn't exists. Using match you might not have a validator even if you expect there to be one due to a typo for example.

@codeclown
Copy link
Contributor Author

Very good point and one that I did not properly consider before. Now I would actually prefer to change the default behaviour so that an exception is thrown by match() if no validator was found for the request (could be disabled via option i.e. validator.match({ allowNoMatch: true })).

What do you think of this @Hilzu?

In regards to this PR... I'll update the docs with current state.

@Hilzu Hilzu merged commit b330e2a into Hilzu:master Jan 31, 2020
@Hilzu
Copy link
Owner

Hilzu commented Jan 31, 2020

That might be one way of doing it.

I'm also thinking about adding a validate-like method, that needs to be explicitly added to routes but wouldn't need the operation specified. Instead it would find the validator using the same machinery as match but could error/warn if one isn't found since you explicitly asked for a validation for a route.

@codeclown
Copy link
Contributor Author

Using match(), if it was changed to throw on unmatched route, that would be the behaviour if you used it on individual routes as well, like this:

app.get('/foo', validator.match(), (req, res, next) => ...);

But I think you're not talking about middleware? Could you illustrate with an example what you mean exactly?

@Hilzu
Copy link
Owner

Hilzu commented Jan 31, 2020

No middleware is exactly what I'm talking about. Somehow I didn't realize that match could also be used like that.

I think making match throw by default and also documenting the usage with a single route is a good way forward.

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.

3 participants