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

Organization with blocks of methods #1487

Open
tibbon opened this issue Sep 10, 2016 · 16 comments
Open

Organization with blocks of methods #1487

tibbon opened this issue Sep 10, 2016 · 16 comments
Labels

Comments

@tibbon
Copy link

tibbon commented Sep 10, 2016

Is there a simple way, to organize the methods with blocks for code folding, organization, etc?

What I'm thinking of using a short version of the example in the README, but wrapping methods and blocks in group blocks:

module Twitter
  class API < Grape::API
    version 'v1', using: :header, vendor: 'twitter'
    format :json
    prefix :api

    resource :statuses do
      group :public_timeline do
        desc 'Return a public timeline.'
        get :public_timeline do
          Status.limit(20)
        end
      end

      group :home_timeline do
        desc 'Return a personal timeline.'
        get :home_timeline do
          authenticate!
          current_user.statuses.limit(20)
        end
      end

      group :status do
        desc 'Return a status.'
        params do
          requires :id, type: Integer, desc: 'Status id.'
        end
        route_param :id do
          get do
            Status.find(params[:id])
          end
        end
      end
    end
  end
end

I'm finding the only way I can get this right now is to use Ruby's open classes, and then having a separate definition for each one. It feels weird for desc and params to not be block based, but rather order based.

@connorshea
Copy link

I'm also interested in this as it seems pretty messy to have these separated.

@dblock
Copy link
Member

dblock commented Sep 13, 2016

We've always thought of desc or params as "annotations". What would be a better syntax?

@connorshea
Copy link

See also: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6330/diffs

You end up with things like this:

module API
  # Projects variables API
  class Variables < Grape::API
    before { authenticate! }
    before { authorize! :admin_build, user_project }

    resource :projects do
      desc 'Get project variables' do
        success Entities::Variable
      end
      params do
        requires :id, type: Integer, desc: 'The ID of a project'
        optional :page, type: Integer, desc: 'The page number for pagination'
        optional :per_page, type: Integer, desc: 'The value of items per page to show'
      end
      get ':id/variables' do
        variables = user_project.variables
        present paginate(variables), with: Entities::Variable
      end

      desc 'Get specific variable of a project' do
        success Entities::Variable
      end
      params do
        requires :id, type: Integer, desc: 'The ID of a project'
        requires :key, type: String, desc: 'The key of the variable'
      end
      get ':id/variables/:key' do
        key = params[:key]
        variable = user_project.variables.find_by(key: key.to_s)

        return not_found!('Variable') unless variable

        present variable, with: Entities::Variable
      end

      desc 'Create a new variable in project' do
        success Entities::Variable
      end
      params do
        requires :id, type: Integer, desc: 'The ID of a project'
        requires :key, type: String, desc: 'The key of the variable'
        requires :value, type: String, desc: 'The value of the variable'
      end
      post ':id/variables' do
        variable = user_project.variables.create(key: params[:key], value: params[:value])

        if variable.valid?
          present variable, with: Entities::Variable
        else
          render_validation_error!(variable)
        end
      end

      desc 'Update existing variable of a project' do
        success Entities::Variable
      end
      params do
        requires :id, type: Integer, desc: 'The ID of a project'
        optional :key, type: String, desc: 'The key of the variable'
        optional :value, type: String, desc: 'TNew value for `value` field of the variable'
      end
      put ':id/variables/:key' do
        variable = user_project.variables.find_by(key: params[:key].to_s)

        return not_found!('Variable') unless variable

        attrs = attributes_for_keys [:value]
        if variable.update(attrs)
          present variable, with: Entities::Variable
        else
          render_validation_error!(variable)
        end
      end

      desc 'Delete existing variable of a project' do
        success Entities::Variable
      end
      params do
        requires :id, type: Integer, desc: 'The ID of a project'
        requires :key, type: String, desc: 'The key of the variable'
      end
      delete ':id/variables/:key' do
        variable = user_project.variables.find_by(key: params[:key].to_s)

        return not_found!('Variable') unless variable
        variable.destroy

        present variable, with: Entities::Variable
      end
    end
  end
end

Without careful spacing it's not immediately obvious which descriptions/parameters are related to one another.

@tibbon
Copy link
Author

tibbon commented Sep 14, 2016

@connorshea That's exactly my problem. I've got a pretty big codebase, and it gets messy like that quickly.

@tibbon
Copy link
Author

tibbon commented Sep 14, 2016

To be clear, I don't want it to modify the path (like an additional resources block would do). I just want something that would wrap everything in a relatively meaningless block for organization, code folding, etc.

@connorshea
Copy link

Exactly the same for me, would be ideal to have a wrapper block to organize things together.

@jason-uh
Copy link

We have created a directory structure that avoids the big block mess and has worked quite well for us. We put each discrete endpoint in its own file and include that into a main API file (e.g. api/v1/samples.rb. See this gist for a closer look.

The directory structure is something like the following. We currently have 14 categories of endpoints (e.g. apples, bananas, and samples in the tree below), each with their own set of discrete actions for a mostly RESTful API. This organizational structure supports nested resources as well as top-level resources. The biggest file we have within this structure is 175 lines long (including blank lines for visual queues) -- that file is a helpers.rb for an endpoint that has a lot of customizable behavior. As far as the endpoint files themselves, the average number of lines is 49, the min is 27 lines, and the max is only 73 lines.

Our params and desc blocks don't get out of control, and (as @dblock mentions) we see them basically as annotations.

I think this type of organization helps with code reading, maintenance, consistency, and a small host of other things. As for code folding, well the file browser of your favorite editor will essentially do that for you. I hope this helps.

(Disclaimer: the names of (most) files and directories have been altered to protect the guilty.)

.../api
├── base.rb
└── v1
    ├── apples.rb
    ├── bananas.rb
    ├── authentication.rb
    ├── base.rb
    ├── concerns
    │   ├── apples
    │   │   ├── helpers.rb
    │   │   └── listing.rb
    │   ├── bananas
    │   │   ├── creating.rb
    │   │   ├── fetching.rb
    │   │   ├── helpers.rb
    │   │   ├── listing.rb
    │   │   └── updating.rb
    │   ├── samples
    │   │   ├── creating.rb
    │   │   ├── deleting.rb
    │   │   ├── fetching.rb
    │   │   ├── helpers.rb
    │   │   ├── listing.rb
    │   │   └── updating.rb
    │   └── zoinkable.rb
    ├── defaults.rb
    ├── error_formatter.rb
    ├── ...
    └── samples.rb

@dblock
Copy link
Member

dblock commented Sep 14, 2016

Both @tibbon and @connorshea raise valid concerns. Also @connorshea should contribute to "who uses grape" at http://www.ruby-grape.org/users/ :)

I do like what @jason-uh is doing. I prefer mount to a block.

That said, I am open to a meaningless block that doesn't modify path, if we can find a good name for it. I suggest endpoint do ... Note that you can try it by just adding a method to Grape::API that yields and see how it feels.

@connorshea
Copy link

@dblock we're already listed under Examples, should we do both? :)

@dblock
Copy link
Member

dblock commented Sep 14, 2016

It doesn't hurt thanks @connorshea! I wonder if we should combine examples and users or reference one to another.

@connorshea
Copy link

@tibbon any progress/thoughts on what'd work best as a name for these blocks?

@connorshea
Copy link

I like the group do syntax, since delete path/to/endpoint do is kind of repetitive of an endpoint block.

Another solution might be to put the description and params inside the method block, though I can understand why you wouldn't (or couldn't?) want to do that.

@ZJvandeWeg
Copy link

On the GitLab-CE issue tracker we've discussed this too, as @connorshea mentioned. We were thinking about a block called endpoint but we'd love to have an official block, probably whatever its called :)

@tibbon
Copy link
Author

tibbon commented Nov 17, 2016

endpoint sounds ok to me, but naming is hard :)

@blackst0ne
Copy link

Hi @dblock
Is there any progress? 🙂
Is there anything I can do to help get this issue done? 🙂

@dblock
Copy link
Member

dblock commented Sep 5, 2017

Feel free to implement and PR something, @blackst0ne.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants