search

pksunkara / alpaca

Given a web API, Generate client libraries in node, php, python, ruby
Mozilla Public License 2.0
2.45k stars 89 forks source link

Fixes to Ruby template #55

Closed mikecarroll closed 9 years ago

mikecarroll commented 9 years ago

One of the Faraday methods in the generated Ruby template was wrong, this PR fixes it. Also updates conditionals in case the API doesn't return a "content-type" header.

pksunkara commented 9 years ago

Let me test this tomorrow and get back to you.

mikecarroll commented 9 years ago

No rush. Appreciate the quick response!

mikecarroll commented 9 years ago

Okay, I realized that the "no method" error wasn't a problem with the template, but an issue with building the gem locally on my end -- I think that, because the gem dependencies in the gemspec were "open ended" bundler was referencing a version of Faraday as a dependency that I had installed locally and which was causing the no method failures.

I updated the dependency declarations to not be open ended (and better in line with best practices: http://guides.rubygems.org/specification-reference/), which resolved that bug for me.

Additionally, I added a version.rb, which is usually the best practice with Ruby gems.

pksunkara commented 9 years ago

Shouldn't you be including the version file somewhere?

mikecarroll commented 9 years ago

Oh, I forgot to commit the file version change. Will push in a moment.

The Ruby best practice is to use && / || (instead of and / or) due to order of precedence issues with and/or that can sometimes cause unexpected behavior. Here's a link on a widely-used Ruby style guide: https://github.com/bbatsov/ruby-style-guide#no-and-or-or

Still, I should have asked before making those changes -- it's habit for me. If you want I can either: (a) Go through and change all the and/or to &&/II in the templates, for consistency. (b) Just revert to using "and" in those statements.

I prefer and am happy to do (a), but defer to your preferences.

pksunkara commented 9 years ago

I would like to go with (b) for now. I do want to go through all the statements later and check if there is going to be any problem, but that is for later. Thanks for bringing this issue up to me.

mikecarroll commented 9 years ago

Give me just a few days to get to this -- on the road now and Internet is touch and go.

mikecarroll commented 9 years ago

That should do it. Sorry it took so many commits to do so little.

pksunkara commented 9 years ago

Merged this manually after squashing some commits. Thanks for the help Mike. :smile: