-
Notifications
You must be signed in to change notification settings - Fork 463
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
Instead of manually redefine the route for the action, retrieve it from the routes of the application. #187
Conversation
Hi, You are my hero! Having multiple routes to one method is the case quite often. What about having also
that would add all the routes for the method? This way, the devs have option if one route is |
Also, maybe we could do this api "Create user" Since the method and path were not defined, we would load them from the routes. What about that? |
Great feature, thanks for contribution 👍. We have been thinking about this since the beginning. |
Thank you so much guys! @iNecas I'm trying to add all the routes for one action, but I'm not sure how should I do, any thoughts ? https://github.com/Pajk/apipie-rails/pull/187/files#diff-3794160d9a3652a5e62136ca6ebc4f91R326 Do you see an inconvenient to add all routes for an action by default ? |
@iNecas for now, I removed the work in progress to support multiple routes for one action. I prefer to do it in another PR once this one will be merged (and used by people ?). |
Tests failing for Rails 3.0.0, do we really want to support a deprecated Rails version ? |
We can at least try figure out what's won't be working. So we can put into the tests condition on the rails version, something like: unless Rails::VERSION::STRING < '3.2.0'
end So we would be able to say at least, that specific features doesn't work with old rails. I have not hard requirement on supporting rails 3.0, but since so far apipie was working on it, some folks might consider it as advantage. |
in ideal case, we would go with deprecation warning with the next release (I would say it's time for |
So i'll go for |
👍 |
I've checked the PR, hit some issues with Sprockets routes (fixed), also looked into possibility of loading all matching routes and using What do you think about #194 ? |
Thanks, I'm checking this now. |
#194 looks good for me 👍 |
@iNecas what do you think about the state of this feature ? (ie: is it mergeable?) |
Hi, I have maybe a bit more flexible way how to define the customization in loading the routes for the "end user", I hope to get that in code sometimes this week. Sry for delay! |
@iNecas sorry to bother you, what are your last thoughts about this PR? I'm willing to help on this side so let me know :) |
method.to_s == route.defaults[:action] | ||
end | ||
|
||
routes.map do |route| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the routes_path_formatter
(or routes_formatter
) shouldn't be a class/object, that would take the array of routes and return the array of the {path: path, verb: '...'}. The reason is the case, whe you would like for some reason for example change the order of api keywords.
What about the default formater class similar to this:
class RoutesFormater
def format_paths(paths)
paths.map { |path| format_path(path) }
end
def format_path(path)
{ path: "", verb: "" }
end
end
It would allow to:
- customize the order of the routes if needed
- skip some routes from getting to the documentation
- add things like automatic description of the api, if needed
What do you think?
@mtparet I really appreciate your patience. Thanks for reviving this PR. One final thought about the interface for the custom formatter. |
@mtparet I hope I didn't put you off just yet… |
@iNecas nope, I was on vacation these last weeks and has to catch up some works this week. I hope to have time next week to finalize this one :) |
d2ce160
to
59febee
Compare
Just rebased and introduced routes formatter class. |
_apipie_dsl_data[:api_args] << [method, path, desc, options] | ||
_apipie_dsl_data[:api] = true | ||
case args.size | ||
when 0..1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we introduced another argument options
, if we want to allow pass it here, we should check size between 0 and 2 but this overlaps with the below check between 2 and 4.
So it means we should use another way to distinguish the two possibility to define routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, if api!
would not make that difference easier: so that we would not be limited on the number of arguments
It's shaping really nicely, some comments inline |
f293ea1
to
2755b11
Compare
(from route or not) so for now, we can't use options when using routes.rb definition
e9bdaa2
to
11ce9cc
Compare
just rebased against master |
@@ -0,0 +1,39 @@ | |||
class RoutesFormater |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be namespaced under Apipie module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtparet no action needed: I will fix that with some other updates I will probably have when testing against my apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I forgot to encapsulate it under the module.
Ability to specify the formatter thought the configuration and influence the other +api+ paramters (such as desc) form the formatter.
Adding a recursion for getting routes form mounted engines. Also checking if we need to add the api_base_url to the path instead of removing the api_base_url when loading the path from routes: doesn't play well with inheritance of api_base_url.
@mtparet I've opened a PR into your branch with my changes ifeelgoods#2, feel free to commend on them. After getting those changes in, I'm ok with merging this into master and releasing 0.3.0 next week. |
Thanks a lot @iNecas, I'll take a look and merge it in my branch. |
Ifeelgoods generate api route
Merging now! Thanks @mtparet for all the work and patience. I expect to release 0.3.0 sometimes next week. |
Instead of manually redefine the route for the action, retrieve it from the routes of the application.
Nice! |
I'm happy to announce that the new version 0.3.0 was released, including this change. Thanks for making that happen https://github.com/Apipie/apipie-rails/blob/master/CHANGELOG.md#v030 |
Description
Instead of manually redefine the route for the action, retrieve it from the routes of the application.
This enable also to have only place where to maintain the routing.
Example:
Instead of defining the name, HTTP verb, and route of the action.
You could now just define the name of the action.
HTTP verb and route will be retrieved from the routes of the application.