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

Instead of manually redefine the route for the action, retrieve it from the routes of the application. #187

Merged
merged 22 commits into from
Dec 18, 2014

Conversation

mtparet
Copy link
Contributor

@mtparet mtparet commented Jan 23, 2014

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.

api :POST, "/users", "Create user"

You could now just define the name of the action.

api! 'Create user'

HTTP verb and route will be retrieved from the routes of the application.

@iNecas
Copy link
Member

iNecas commented Jan 23, 2014

Hi, You are my hero!

Having multiple routes to one method is the case quite often. What about having also

api_routes desc

that would add all the routes for the method? This way, the devs have option if one route is
enough, all they want all of them. Thoughts?

@iNecas
Copy link
Member

iNecas commented Jan 23, 2014

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?

@Pajk
Copy link
Member

Pajk commented Jan 23, 2014

Great feature, thanks for contribution 👍. We have been thinking about this since the beginning.

@mtparet
Copy link
Contributor Author

mtparet commented Jan 23, 2014

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 ?

@mtparet
Copy link
Contributor Author

mtparet commented Jan 24, 2014

@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 ?).

@mtparet
Copy link
Contributor Author

mtparet commented Jan 27, 2014

Tests failing for Rails 3.0.0, do we really want to support a deprecated Rails version ?

@iNecas
Copy link
Member

iNecas commented Jan 27, 2014

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.

@iNecas
Copy link
Member

iNecas commented Jan 27, 2014

in ideal case, we would go with deprecation warning with the next release (I would say it's time for 0.1.0 version finally :) and remove the support next version (I don't have issues with release 0.2.0 quite soon, just to drop the support.

@mtparet
Copy link
Contributor Author

mtparet commented Jan 27, 2014

So i'll go for Rails::VERSION::STRING < '3.2.0' :)

@AlekSi
Copy link
Contributor

AlekSi commented Jan 28, 2014

👍

@iNecas
Copy link
Member

iNecas commented Jan 28, 2014

I've checked the PR, hit some issues with Sprockets routes (fixed), also looked into possibility of loading all matching routes and using api keyword when not specifying the vert/path.

What do you think about #194 ?

@mtparet
Copy link
Contributor Author

mtparet commented Jan 28, 2014

Thanks, I'm checking this now.

@mtparet
Copy link
Contributor Author

mtparet commented Jan 28, 2014

#194 looks good for me 👍

@mtparet
Copy link
Contributor Author

mtparet commented Feb 28, 2014

@iNecas what do you think about the state of this feature ? (ie: is it mergeable?)

@iNecas
Copy link
Member

iNecas commented Mar 3, 2014

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!

@mtparet
Copy link
Contributor Author

mtparet commented Jul 26, 2014

@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|
Copy link
Member

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:

  1. customize the order of the routes if needed
  2. skip some routes from getting to the documentation
  3. add things like automatic description of the api, if needed

What do you think?

@iNecas
Copy link
Member

iNecas commented Jul 26, 2014

@mtparet I really appreciate your patience. Thanks for reviving this PR. One final thought about the interface for the custom formatter.

@iNecas
Copy link
Member

iNecas commented Aug 29, 2014

@mtparet I hope I didn't put you off just yet…

@mtparet
Copy link
Contributor Author

mtparet commented Aug 29, 2014

@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 :)

@mtparet
Copy link
Contributor Author

mtparet commented Nov 4, 2014

Just rebased and introduced routes formatter class.
(see comment about one questioning)

_apipie_dsl_data[:api_args] << [method, path, desc, options]
_apipie_dsl_data[:api] = true
case args.size
when 0..1
Copy link
Contributor Author

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.

Copy link
Member

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

@iNecas
Copy link
Member

iNecas commented Nov 10, 2014

It's shaping really nicely, some comments inline

@mtparet
Copy link
Contributor Author

mtparet commented Dec 8, 2014

just rebased against master

@@ -0,0 +1,39 @@
class RoutesFormater
Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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.
@iNecas
Copy link
Member

iNecas commented Dec 17, 2014

@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.

@mtparet
Copy link
Contributor Author

mtparet commented Dec 17, 2014

Thanks a lot @iNecas, I'll take a look and merge it in my branch.

@iNecas
Copy link
Member

iNecas commented Dec 18, 2014

Merging now! Thanks @mtparet for all the work and patience. I expect to release 0.3.0 sometimes next week.

iNecas added a commit that referenced this pull request Dec 18, 2014
Instead of manually redefine the route for the action, retrieve it from the routes of the application.
@iNecas iNecas merged commit 4627d69 into Apipie:master Dec 18, 2014
@mtparet
Copy link
Contributor Author

mtparet commented Dec 18, 2014

Nice!

@iNecas
Copy link
Member

iNecas commented Jan 7, 2015

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

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