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

Ensure path parameters are declared in each Operation #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbuhot
Copy link
Collaborator

@mbuhot mbuhot commented Apr 20, 2019

Paths.from_router now returns an :ok/:error tuple.
The error case will occur if any operation fails to declare a path parameter that is present in the phoenix route.

Fixes #52

@fenollp Is a breaking API change the way to go for this one? We could alternatively let it raise an error without changing the return type of the function.

@mbuhot mbuhot requested a review from fenollp April 20, 2019 04:37
Copy link
Contributor

@moxley moxley left a comment

Choose a reason for hiding this comment

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

Looks great. It would be preferred to keep Plug syntax for route paths, and to use Plug's built-in path parsing function, but I understand if that's not feasible.

@fenollp
Copy link
Contributor

fenollp commented Apr 23, 2019

I agree with @moxley that this should defer to Plug for path parsing.
Also I’d say this doesn’t warrant a major version bump since this is a bugfix. I think the API should stay as before and just throw when encountering unexpected paths.

This would fail at compile time, right?

@moxley
Copy link
Contributor

moxley commented Apr 23, 2019

@fenollp:

Also I’d say this doesn’t warrant a major version bump since this is a bugfix.

It's a breaking API change. The bug doesn't seem directly related to the part of the API that has changed. But I don't know. I'm no expert at determining what constitutes a major version bump.

Perhaps a new function could be introduced that has the new behavior, and the old function could raise a warning?

@mbuhot mbuhot force-pushed the fix/ensure-path-params-declared branch from 9bfc284 to a9e3554 Compare April 28, 2019 06:48
Paths.from_router will raise an exception if an operation fails to
declare a path parameter that exists in the route
@mbuhot mbuhot force-pushed the fix/ensure-path-params-declared branch from a9e3554 to d61d03f Compare April 28, 2019 07:10
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.

Missing path parameters go unnoticed
3 participants