Fix version prefix in specs [WIP] #54

Open
wants to merge 7 commits into
from

Projects

None yet

3 participants

@keithpitt
Contributor

I've got a failing spec, but I'm struggling with the fix.

I can see that parameter defaults aren't getting passed through to the controller, and thus the prefix removal stuff isn't working.

I don't understand how it can actually work with Rails, but fail for a spec yet.

I've also cleaned up the rspec have_exposed error messages. They were a bit funky, and now they make a bit more sense.

@keithpitt
Contributor

@Sutto if you can think of anything I could try, let me know.

@keithpitt
Contributor

I've spent some more time trying to debug this one. So if we don't use defaults to pass though the version prefix, everything is peachy. Its when we use defaults, then things go crazy. I think I'm close to giving up on this one for a while. I've got deep into the bowels of action_pack and journey and I still havent't found an answer yet.

@keithpitt
Contributor

So I think I have a solution. Why do you need to re-verify the version? What if we just didn't pass the prefix data as defaults. Why does the controller need that? I'm going to try to remove that stuff, and see what happens.

@keithpitt
Contributor

@Sutto check out my latest commit. Integration specs pass. Required/optional version prefixes are handled through the version constraint at the routing level - so I don't see a need for us to re-check it down in rocket-pants. Versions are numbers - so we can just strip anything thats not a number. If you're ok with approach - I'll fix up the unit tests.

@Sutto
Owner
Sutto commented Mar 5, 2013

@keithpitt Interesting - as long as #39 stays solved, we should be ok. Will take a proper look at the code shortly.

@keithpitt
Contributor

I'm not sure what the issues is with that ticket. Is there a spec that addresses his issue that I should be looking at?

@Sutto
Owner
Sutto commented Mar 6, 2013

They key part of this is that we must:

A) support correctly generating routes with and without the version prefix in the controller - require should include, allow and others likely not?
B) We should be able to simply handle parsing them in the controller. I think your comment about strip anything but a number mostly makes sense, and is simple enough for the moment. I would probably go the opposite: get the first number from the string and use that.

@Sutto
Owner
Sutto commented Mar 6, 2013

See master...fix-version-prefix-in-specs for where I started merging this.

@Sutto
Owner
Sutto commented Mar 6, 2013

The big issue remaning is invalid versions, but I think that's a routing concern.

@keithpitt
Contributor

Yeah - I think it is as well. Is it just unit tests left to get this into master?

@fabn
Contributor
fabn commented Mar 20, 2013

Hi all, I've started again to work on my project which uses RP. I've added version prefix support some months ago and now I see that I broke something with that patch :-(

My original project which inspired my for that proposal was migrated from grape to rocket pants and I've implemented that patch to keep it backward compatible. I've used my patch in a spec like this one

require 'spec_helper'

describe VersionsController, :focus do

  default_version 'v2'

  it "should provide version" do
    get :index
    response.body.should be_json_eql({version: _default_version, full_version: APP_VERSION}.to_json)
  end

end

But while this kind of spec works with my original work (i.e. 676379b) it has started to fail with b7c8443

Now reading this issue I see that with this version of the gem https://github.com/keithpitt/rocket_pants/tree/fix-version-prefix-in-specs almost everything works again (I still have a couple of failing specs, need to investigate if they are related to this).

I'd like to help to solve this, so I'll try to dig into that. Let me know if there is something I can do.

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