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.

Sign up for GitHub

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

Jump to bottom

Better resource name #32

Closed
wants to merge 3 commits into from
Closed

Better resource name #32

wants to merge 3 commits into from

Conversation

aselder
Copy link

@aselder aselder commented Jul 13, 2020

After talking with Jose about a better general solution to this problem, here's what we came up with.

aselder added 3 commits July 9, 2020 09:17
@aselder
Better calculate the route for the resource name
c9885a6 
Right now, the calculcation of the routes strips static
parts of the path when calculating the resource name

For instance if you have the following route:

```
scope "/carts" do
  forward "/cart", Routers.CartRouter
end
```

and in the CartRouter
```
scope "/", ApiV1Web do
  get "/:app_id/:identifier", CartController, :get
end
```

the request path would be something like `/carts/cart/1234/abc`. This
would get send with a resource name of `GET /:app_id/:identifier`, which
make is very hard to differentiate between routes in a different
router or scope.

This change preserves the static parts of the route so that a clearer
route name is obtained.
@aselder
Appease the format cop.
980bb3c
@aselder
Better solution after talking to Jose.
21702d6
@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 fixed issue! 🎉

But beware that this branch is 2 commits behind the spandex-project:master branch, and a review of an up to date branch would produce more accurate results.

See more details about this review.

@zachdaniel
Copy link
Member

Nice! I didn't know that utility existed. Is it in a newer version of phoenix? If so, we may want to check for its existence with :erlang.function_exported/3 or something similar, and use the old method if it doesn't exist.

@aselder
Copy link
Author

aselder commented Jul 13, 2020

Let me do dig and figure out when that route_info function was introduced.

@aselder
Copy link
Author

aselder commented Jul 13, 2020

Based on hexdocs.pm, it looks like route_info was introduced Phoenix 1.4.7, which is probably too recent to count on existing. I'll add in the fallback on the old method if it doesn't exist.

@zachdaniel
Copy link
Member

This looks good otherwise, will merge once that is done.

@zachdaniel
Copy link
Member

@aselder its been a while since this PR was opened, and the backup method we discussed hasn't been added, so I'm going to close this for now.

@zachdaniel
Copy link
Member

Feel free to reopen/add the other method.

@zachdaniel zachdaniel closed this Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aselder @zachdaniel