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

introducing :collection and :member options to simplify the use of :map #1733

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@goncalvesjoao

goncalvesjoao commented Aug 5, 2014

Hi guys!
First of all. Thanks for all the great work, I'm loving padrino so far and it is a must for quickly bootstrapping new micro-services projects.

I've been trying to simplify my controllers' code in order to please the almighty codeclimate god and for readability purposes I realised that in some cases, the use of 'collection' and 'member' for routes (like rails) might make sense.

Here's an old controller of mine:

module V0
  module Medicines

    COLLECTION_URL = "v0/medicines"

    MEMBER_URL = "#{COLLECTION_URL}/:id"

    MedicationLocker::App.controllers COLLECTION_URL do

      get :index do
        ...
      end

      get :prescribed_by_medic, map: "#{COLLECTION_URL}/prescribed_by_medic/:medic_id" do
        ...
      end

      get :show, map: MEMBER_URL do
        ...
      end

      post :bulk_create, map: "#{COLLECTION_URL}/bulk_create" do
        ...
      end

      post :create, map: COLLECTION_URL do
        ...
      end

      put :update, map: MEMBER_URL do
        ...
      end

      put :update_chronic_state, map: "#{MEMBER_URL}/chronic_state" do
        ...
      end

      delete :stop_treatment, map: "#{MEMBER_URL}/stop_treatment" do
        ...
      end

      delete :destroy, map: MEMBER_URL do
        ...
      end

    end

  end
end

and the new version with 'collection' and 'member' options (kinda like rails routes)

module V0
  module Medicines

    MedicationLocker::App.controllers "v0/medicines" do

      get :index do
        ...
      end

      get :prescribed_by_medic, collection: "prescribed_by_medic/:medic_id" do
        ...
      end

      get :show, member: "" do
        ...
      end

      post :bulk_create, collection: "bulk_create" do
        ...
      end

      post :create, collection: "" do
        ...
      end

      put :update, member: "" do
        ...
      end

      put :update_chronic_state, member: "chronic_state" do
        ...
      end

      delete :stop_treatment, member: "stop_treatment" do
        ...
      end

      delete :destroy, member: "" do
        ...
      end

    end

  end
end

Both versions will lead to

Application: MedicationLocker::App
    URL                                             REQUEST  PATH
    (:v0/medicines, :index)                           GET    /v0/medicines
    (:v0/medicines, :prescribed_by_medic)             GET    /v0/medicines/prescribed_by_medic/:medic_id
    (:v0/medicines, :show)                            GET    /v0/medicines/:id
    (:v0/medicines, :bulk_create)                    POST    /v0/medicines/bulk_create
    (:v0/medicines, :create)                         POST    /v0/medicines
    (:v0/medicines, :update)                          PUT    /v0/medicines/:id
    (:v0/medicines, :update_chronic_state)            PUT    /v0/medicines/:id/chronic_state
    (:v0/medicines, :stop_treatment)                DELETE   /v0/medicines/:id/stop_treatment
    (:v0/medicines, :destroy)                       DELETE   /v0/medicines/:id

The new changes are quite simple and I've also added tests for them.
All original tests are green and so do the new ones.

I believe this might be useful for others and it would make my day to contribute my '50cents' to padrino.

PS: I haven't change the version number nor the documentation about these new changes, don't wanna mess anything up.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 9, 2014

Member

This looks extremely cryptic.

How should this code explain itself? What does it do?

      delete :stop_treatment, member: "stop_treatment" do
        ...
      end
      post :create, collection: "" do
        ...
      end

@padrino/core-members ?

Member

ujifgc commented Aug 9, 2014

This looks extremely cryptic.

How should this code explain itself? What does it do?

      delete :stop_treatment, member: "stop_treatment" do
        ...
      end
      post :create, collection: "" do
        ...
      end

@padrino/core-members ?

@goncalvesjoao

This comment has been minimized.

Show comment
Hide comment
@goncalvesjoao

goncalvesjoao Aug 9, 2014

I does the same as in Rails.

In rails routes, one would do

namespace :v0 do
  resources :medicines, except: [:new, :edit] do
    collection do
      get 'prescribed_by_medic/:medic_id'
      post 'bulk_create'
    end
    member do
      put 'chronic_state' => 'medicines#update_chronic_state'
      delete 'stop_treatment'
    end
  end
end

to generate the exact same routes as described above.

I've been extracting code from a Rails monolithic app into micro-services and the concept of 'collection' and 'member' options came from that.

Although if you found it cryptic, then it must be confusing for someone else too.

Being this approach somewhat confusing, I would suggest that :map (if not an absolute path) should be relative to the controller's path/name.

Since padrino introduces the concept of controllers in sinatra, I was kinda expecting that defining a route inside a controller

MedicationLocker::App.controllers "v0/medicines" do
  get :show, map: ":id" do
    ...
  end
end

that the route's path would be suffixed with the controller's path, generating something like:

Application: MedLocker::App
    URL                                             REQUEST  PATH
    (:v0/medicine_interactions, :show)                GET    /v0/medicine_interactions/:id

and for instance a :map with an absolute path

MedicationLocker::App.controllers "v0/medicines" do
  get :show, map: "/:id" do
    ...
  end
end

would override the controller's path.

Application: MedLocker::App
    URL                                             REQUEST  PATH
    (:v0/medicine_interactions, :show)                GET    /:id

The problem is that altering :map's behaviour will certainly break many people's code.

Hence this, I added for myself these two new route options without breaking the usage of :map.
Later I realised that maybe someone else from Rails background might relate to it and made a pull-request.

If you guys find the first approach cryptic, then it probably is for many other people.
Using it heavily in Rails I assumed other's would relate to.

Well, bottom line, maybe you can include this feature in future version's of padrino, a way to define a route's path relative to the controller's name.

@padrino/core-members

Sincerely,
John

goncalvesjoao commented Aug 9, 2014

I does the same as in Rails.

In rails routes, one would do

namespace :v0 do
  resources :medicines, except: [:new, :edit] do
    collection do
      get 'prescribed_by_medic/:medic_id'
      post 'bulk_create'
    end
    member do
      put 'chronic_state' => 'medicines#update_chronic_state'
      delete 'stop_treatment'
    end
  end
end

to generate the exact same routes as described above.

I've been extracting code from a Rails monolithic app into micro-services and the concept of 'collection' and 'member' options came from that.

Although if you found it cryptic, then it must be confusing for someone else too.

Being this approach somewhat confusing, I would suggest that :map (if not an absolute path) should be relative to the controller's path/name.

Since padrino introduces the concept of controllers in sinatra, I was kinda expecting that defining a route inside a controller

MedicationLocker::App.controllers "v0/medicines" do
  get :show, map: ":id" do
    ...
  end
end

that the route's path would be suffixed with the controller's path, generating something like:

Application: MedLocker::App
    URL                                             REQUEST  PATH
    (:v0/medicine_interactions, :show)                GET    /v0/medicine_interactions/:id

and for instance a :map with an absolute path

MedicationLocker::App.controllers "v0/medicines" do
  get :show, map: "/:id" do
    ...
  end
end

would override the controller's path.

Application: MedLocker::App
    URL                                             REQUEST  PATH
    (:v0/medicine_interactions, :show)                GET    /:id

The problem is that altering :map's behaviour will certainly break many people's code.

Hence this, I added for myself these two new route options without breaking the usage of :map.
Later I realised that maybe someone else from Rails background might relate to it and made a pull-request.

If you guys find the first approach cryptic, then it probably is for many other people.
Using it heavily in Rails I assumed other's would relate to.

Well, bottom line, maybe you can include this feature in future version's of padrino, a way to define a route's path relative to the controller's name.

@padrino/core-members

Sincerely,
John

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 9, 2014

Member

Okay, the problem is reasonable.

Here's how it could work with minimal API change:

InternationalDrugCartel::App.controller :'v9/medicines' do
  # works as is:
  get :index do; end
  get :prescribed_by_medic, :with => [ :medic_id ] do; end
  # works as is but needs a test to lock this feature:
  get :show, '', :with => [ :id ] do; end
  # needs cosmetic patch to pass verb arguments to Router around Sinatra:
  post :bulk_create do; end
  post :create, '' do; end
  put :update, '', :with => [ :id ] do; end
  # needs small patch to :with syntax to allow constant strings:
  put :update_chronic_state, '', :with => [ :id, 'update_chronic_state' ] do; end
  delete :stop_treatment, '', :with => [ :id, 'stop_treatment' ] do; end
  delete :destroy, '', :with => [ :id ] do; end
end

=> the same route map as yours

    URL                                       REQUEST  PATH
    (:v9/medicines, :index)                     GET    /v9/medicines
    (:v9/medicines, :prescribed_by_medic)       GET    /v9/medicines/prescribed_by_medic/:medic_id
    (:v9/medicines, :show)                      GET    /v9/medicines/:id
    (:v9/medicines, :bulk_create)              POST    /v9/medicines/bulk_create
    (:v9/medicines, :create)                   POST    /v9/medicines
    (:v9/medicines, :update)                    PUT    /v9/medicines/:id
    (:v9/medicines, :update_chronic_state)      PUT    /v9/medicines/:id/update_chronic_state
    (:v9/medicines, :stop_treatment)          DELETE   /v9/medicines/:id/stop_treatment
    (:v9/medicines, :destroy)                 DELETE   /v9/medicines/:id
Member

ujifgc commented Aug 9, 2014

Okay, the problem is reasonable.

Here's how it could work with minimal API change:

InternationalDrugCartel::App.controller :'v9/medicines' do
  # works as is:
  get :index do; end
  get :prescribed_by_medic, :with => [ :medic_id ] do; end
  # works as is but needs a test to lock this feature:
  get :show, '', :with => [ :id ] do; end
  # needs cosmetic patch to pass verb arguments to Router around Sinatra:
  post :bulk_create do; end
  post :create, '' do; end
  put :update, '', :with => [ :id ] do; end
  # needs small patch to :with syntax to allow constant strings:
  put :update_chronic_state, '', :with => [ :id, 'update_chronic_state' ] do; end
  delete :stop_treatment, '', :with => [ :id, 'stop_treatment' ] do; end
  delete :destroy, '', :with => [ :id ] do; end
end

=> the same route map as yours

    URL                                       REQUEST  PATH
    (:v9/medicines, :index)                     GET    /v9/medicines
    (:v9/medicines, :prescribed_by_medic)       GET    /v9/medicines/prescribed_by_medic/:medic_id
    (:v9/medicines, :show)                      GET    /v9/medicines/:id
    (:v9/medicines, :bulk_create)              POST    /v9/medicines/bulk_create
    (:v9/medicines, :create)                   POST    /v9/medicines
    (:v9/medicines, :update)                    PUT    /v9/medicines/:id
    (:v9/medicines, :update_chronic_state)      PUT    /v9/medicines/:id/update_chronic_state
    (:v9/medicines, :stop_treatment)          DELETE   /v9/medicines/:id/stop_treatment
    (:v9/medicines, :destroy)                 DELETE   /v9/medicines/:id
@goncalvesjoao

This comment has been minimized.

Show comment
Hide comment
@goncalvesjoao

goncalvesjoao Aug 9, 2014

Works for me @ujifgc, if that gets to be implemented I rest my case :D

goncalvesjoao commented Aug 9, 2014

Works for me @ujifgc, if that gets to be implemented I rest my case :D

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 9, 2014

Member

Done in #1739

It might get released with 0.12.3

Member

ujifgc commented Aug 9, 2014

Done in #1739

It might get released with 0.12.3

@goncalvesjoao

This comment has been minimized.

Show comment
Hide comment
@goncalvesjoao

goncalvesjoao commented Aug 9, 2014

Thanks man

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment