Added Facet Support #20
Conversation
|
@duwanis Is there anything I can do to help get this PR reviewed? Facet support would be a very nice addition to Asari! |
| @@ -24,6 +24,7 @@ Gem::Specification.new do |s| | |||
| # s.add_runtime_dependency "rest-client" | |||
|
|
|||
| s.add_runtime_dependency "httparty" | |||
| s.add_runtime_dependency "curb" | |||
There was a problem hiding this comment.
@cbilgili Why was curb added as a dep?
|
There was problem so I supposed it because of httparty. I had added to test it. But it was unnecessary, I have removed it. I will be happy if you test it and merge it with gem. |
|
Sweet, thanks @cbilgili! Yesterday I made those same changes to my fork and have been testing it on our staging server. I'll let you and @duwanis know if I find any problems. Hopefully a bit of real-world use will prove this PR works well and can be merged. |
|
Thanks @duwanis! Appreciate you taking the time to comment and I hope things work out and you're able to review this PR. Totally understand you're busy and grateful for when you do have time to help with OSS stuff. (love your blog btw) |
|
Thanks @jwarchol ! I can make revisions if necessary! |
| @@ -5,6 +5,7 @@ | |||
| require "asari/geography" | |||
|
|
|||
| require "httparty" | |||
| require "curb" | |||
There was a problem hiding this comment.
Should this be removed as well if curl is not required?
There was a problem hiding this comment.
Yes you may delete. There was HTTP error so I suspected of httparty. But the problem was long query string to Amazon.
Kaminari pagination
|
Hey guys, I'm helping @wellbredgrapefruit with doing pull requests etc. on this this. I like the idea of adding facets and we always appreciate pull requests. As I write this, if you look at the current unit test code coverage for the project it is at 100%. There is also a broken window in the form of 1 broken unit test on master that I am in the process of fixing along with adding in Travis-CI support. Can you add some unit tests to this so that subsequent changes don't accidentally break this? The implementation code looks good, and once we have some good specs I'll bring this in. |
|
@cbilgili @jwarchol can you add some unit tests to this PR, so @lgleasain can merge it? It would be nice to use it in official repo. |
No description provided.