Define Boolean for Grape::API::Instance

[Bhacaz]

When using the Grape::API::Instance the constant Boolean isn't declare.

Example:

require 'grape'

class EchoAPI < Grape::API::Instance
  version 'v1'
  format :json
  prefix :api

  params do
    requires :message, type: Boolean
  end
  post :echo do
    params[:message]
  end
end

Will raise: main.rb:9:in `block in <class:EchoAPI>': uninitialized constant EchoAPI::Boolean (NameError)

The work around is to use requires :message, type: Virtus::Attribute::Boolean, but it's not very consistent with Grape::API and it was declare has an issue #1144.

Maybe there's a better implementation to fix the issue, like in the Grape::DSL::Helpers (done for the issue here #1153).

[dblock]

Do you think you can make an integration test for this?

[dblock]

Correct the casing of Grape::Api::Instance and quote it. Also quote ValidatorFactory above while we're at it, please.

[dblock]

The test added doesn't test the original problem though, no? This succeeds with or without the code change?

it 'works' do
class EchoAPI < Grape::API::Instance
  params do
    requires :message, type: Boolean
  end
  post :echo do
    params[:message]
  end
end

[Bhacaz]

The spec failed if I remove the line Boolean = Virtus::Attribute::Boolean. The original problem was that Grape::API::Instance doesn't define the constant Boolean. It thing my change fix that and the spec test if the class respond to Grape::API::Instance::Boolean and it's value.

Maybe I don't follow how the specs is usually done in this project. I open to suggestions...

[dblock]

Your spec tests that you define Boolean inside Grape::Api::Instance, not that it actually works for the API developer. In fact, it doesn't. This is failing for me even with your change.

require 'spec_helper'

describe Grape::API::Instance do
  describe 'boolean constant' do
    subject do
  	  Class.new(Grape::API::Instance) do
    	  params do
    	    requires :message, type: Boolean
    	  end
    	  post :echo do
    	    params[:message]
    	  end
      end
  	end
    def app
      subject
    end
    it 'sets Boolean as a Virtus::Attribute::Boolean' do
  	  get '/echo?message=true'
	  ...
    end
  end
end

[Bhacaz]

@dblock First of all, thank you to have taken the time to try/rewrite this spec. I try it and without surprise like you said it failed. So I change Class.new(Grape::API::Instance) do to Class.new(Grape::API) do thinking it will pass and it didn't. So I look around trying to find how to set a API class has the app to make it work with Grape::API and I found spec/grape/validations/validators/allow_blank_spec.rb. With that way it pass with both inheritance. What do you think?

[dblock]

Ok, this works for me, thank you. Can I please nitpick on the spec:

1. Can you please test that it effectively coerces a boolean? Maybe return { class: params[:message].class.name, value: params[:message] } from the API and check that the return is Boolean and true.
2. Let's rename the spec file, instead of instance_spec how about defines_boolean_in_params_spec.rb or something similar?

[dblock]

Nitpick, write a separate spec with this line and put params as subject or let. I'll stop after this I promise :)

[dblock]

I was wondering why this actually worked for Grape::API, and found this. Should we be doing the same thing as that implementation by opening up the Instance class in that file instead?

[Bhacaz]

I saw it but I didn't realize it was in coerce.rb. I really thought I was doing the same thing as Grape::API (my bad). I think we should use the same thing for both classes. Should I put my specs in an other place, spec/grape/validations/validators/coerce_spec.rb for example?

[dblock]

I merged this. Thanks for hanging in there! I think the spec is fine where it is. Feel free to make proposals/suggestions/move things around if you think otherwise!