bellycard / napa

A simple framework for building APIs with Grape
Other
329 stars 72 forks source link

upgrade roar to current version #242

Closed ashtonthomas closed 8 years ago

ashtonthomas commented 8 years ago

We define a subclass of Roar::Decorator and include some modules which have had their namespace changed in roar 1.0.0.

Roar has seen a lot of new developments recently with support for hypermedia attributes. These changes along with the general value of not restricting dependency versions when we don't need to should merit this pull request.

In this pull request:

ashtonthomas commented 8 years ago

We really only need to bump roar to 1.0.0 to get the change to namespaces

darbyfrey commented 8 years ago

Could we change it to support both pre 1.0 and 1.x versions of Roar? Ideally, I think the individual services should decide which version they want to use and Napa should support all versions

ashtonthomas commented 8 years ago

@darbyfrey, is there a pattern for supporting different versions like you suggest?

I used a LoadError rescue and this seems to work. I've updated this pull request to support pre and post namespace refactoring. I have change the gemspec to allow higher version changes. Also, the Coercion didn't need a namespace prefix to work.

I tested this with an internal app and set roar to ~> 0.12.0 (0.12.9) as well as 1.0.0 and HEAD versions of roar.

Thoughts?

darbyfrey commented 8 years ago

While this would work, it's not very clear why you're doing the begin/rescue, etc.

A simple way to do it would be wrap a conditional around the VERSION value. So, something like:

if Roar::VERSION > 'x.y.z'
  # load files
else
  # load other files
end

Are the dependencies clear enough that you could use this approach?

ochagata commented 8 years ago

I agree that it shouldn't cause a LoadError and instead use an if/else clause.

ashtonthomas commented 8 years ago

@darbyfrey I've used your approach. Yeah, that makes perfect sense. I was just totally unsure of the better way to do it. Thanks for the tip.

@ochagata, I've added the pessimistic upper bound limiting roar to 1.x

ochagata commented 8 years ago

:+1:

ashtonthomas commented 8 years ago

@darbyfrey, I removed the constant, but I'm still not sure if this is the most appropriate method

darbyfrey commented 8 years ago

This seems fine to me. You could add a helper method to wrap the version check to be more explicit about what you're doing, but that's not totally necessary.

+1 from me. Anyone else?

shaqq commented 8 years ago

LGTM +1

darbyfrey commented 8 years ago

@ashtonthomas, will you add a note for this change into the CHANGELOG? Then we can merge it in.

ashtonthomas commented 8 years ago

@darbyfrey, sorry for the delay. I rebased with master and added the changelog

darbyfrey commented 8 years ago

Thanks @ashtonthomas!