search

lostisland / faraday_middleware

Various Faraday middlewares for Faraday-based API wrappers
MIT License
557 stars 203 forks source link

Handle empty parser_options more reliably #163

Closed stefansedich closed 7 years ago

stefansedich commented 7 years ago

Based on the discussion in #156 I decided to take the path of least resistance and if there are no parser_options just call .parse without them, I did this instead of defaulting the options to {} to remove any other future surprises.

stefansedich commented 7 years ago

Sorry @iMacTia I pushed some changes and lost the context of the review, had to move to next as returns from the proc do not work, I think I might have already covered your feedback as I had pushed up before I removed the second if.

iMacTia commented 7 years ago

Sorry @stefansedich I couldn't be very specific as I was on mobile. I meant that we won't need this change anymore: https://github.com/lostisland/faraday_middleware/blob/master/lib/faraday_middleware/response_middleware.rb#L52

You added this "if" but we shouldn't need it anymore, I think you can always pass both parameters as on the other side there's a block (which in theory accepts a dynamic number of parameters). Since we now have the || {} fallback we can simplify that part, but please let me know if you have any issue with tests

stefansedich commented 7 years ago

I get it now @iMacTia I removed that IF and we should be good to go, just looking at my original PR you had mentioned:

IMPORTANT: We should pass this parameter ONLY if it's provided. We might have people defining custom parser that doesn't accept the second parameter. In order to maintain backward compatibility we need to make the second parameter optional

Looking at this again I don't think it matters as any block params not used are just ignored so it should be fine, thoughts?

iMacTia commented 7 years ago

Yeah you're right, I came out with that requirement in the first place. I did some additional tests and confirmed that Procs don't have the constrain on the number of parameters. Since we clearly state that define_parser should accept a Proc, than this shouldn't be an issue even for custom parsers