search

bellycard / napa

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

Update test of data to check for the method actually used rather than… #217

Closed leason closed 9 years ago

leason commented 9 years ago

… assume that an object that responds to :to_a can respond to :map.

This resolves https://github.com/bellycard/napa/issues/216

shaqq commented 9 years ago

@leason Sorry for the late response - could you add some specs to confirm this is working as intended?

I'd also be curious why the Mongoid object responds to to_a and not map - it could be something to solve on their end.

leason commented 9 years ago

Shaqq, I am no expert, but I do know that :to_a (http://www.rubydoc.info/github/mongoid/mongoid/Mongoid/Document:to_a) is a helper type method they added that looks like it's just there for convenience. Maybe that's not the best way because maybe the rest of the ruby community sees :to_a as a special method, but I doubt there's much to be done about that now.

To me though, as a newbie reading the napa source, I just question whether it's a safe assumption that just because an object responds to :to_a that it will respond to :map. If you need :map in order to do something, shouldn't we just test for that method?

In any case, I'm trying to get rspec tests to run for Napa and will try to add a spec or two as soon as I can.

shaqq commented 9 years ago

:100:

umtrey commented 9 years ago

+1 here - we're asking for the object to call map immediately after the conditional, so verifying that it responds to this method is best. Mapping does not require the object itself to be directly coercible to an array, just that it can be iterated over as an enumerator.

leason commented 9 years ago

Shaqq, not sure exactly what you were looking for with the spec, but I just pushed a pretty straight forward test. I didn't see any existing tests for the grape helpers other than the Napa::Representable::IncludeNil test. Let me know if you need any more on this PR.

shaqq commented 9 years ago

@leason I was originally looking for tests that would show whether this PR would fix the issue with Mongoid. Though, that's really a much harder thing to test for the representers.

+1 from me

leason commented 9 years ago

@shaqq Ah, gotcha. Yeah, I've no idea how to demonstrate that explicitly. If anyone has specific suggestions I'm happy to try them. Again, very new to Ruby/Rspec and especially to Napa. Thanks for accepting the PR though! Hopefully this will help other Mongo users. Napa + Mongo is actually a really great pairing (if you get my drift... grape... napa... wine pairing... okay, I'll see myself out)

shaqq commented 9 years ago

:wink: