-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hannah C - API Muncher - Octos #46
base: master
Are you sure you want to change the base?
Conversation
…outn of info needed
API MuncherWhat We're Looking For
This submission is concerning to me. I do see lots of things I like here - your test coverage is pretty good, and most of the presentation aspects are working fine. However, you were not able to get an individual recipe from the API, which indicates that the core learning goal for the assignment was missed. This submission is also missing error handling and deployment. Remembering how to write "regular" Ruby code is part of the challenge of this assignment, but it seems like you're still struggling with Ruby fundamentals more than I'd like. Many of the problems I'm seeing are things that will hamper the rest of your development process. For example, having a good story around error handling makes everything else easier, because when something goes wrong you get a clear reason why instead of having to hunt for it. If it's alright with you, I'm going to talk to Charles about scheduling you for a tutoring session or two. You've had several submissions in a row that aren't where we want them to be, and I think it would be worthwhile to shore up your fundamentals a bit as we head into JavaScript. I would start by looking over the broken API interactions and error handling and getting that to work really well. Error handling is also something that you'll have plenty of opportunities to practice on VideoStore API. I know you've been putting in a ton of effort recently, and I want to acknowledge that. I do see improvements happening and progress being made - testing in particular is in a much better spot than it was. Keep up your focus and dedication, and things will fall into place. |
|
||
response = HTTParty.get(full_url) | ||
|
||
raise_on_error(response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use the same error handling function for both endpoints, but the form of the response is different, so this ends up breaking.
|
||
def show | ||
@recipe = EdamamApiWrapper.display_recipe(params[:uri]) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use resources
for this route, which means that you're looking for params[:id]
, not params[:uri]
.
def index | ||
recipes = EdamamApiWrapper.list_recipes(params[:query]) | ||
|
||
@recipes = Kaminari.paginate_array(recipes).page(params[:page]).per(12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you expect the lib file to raise an error, you should wrap this call in a begin
/rescue
.
def initialize(label, image, uri, url, ingredient_list, health_labels, source) | ||
valid_attribute?(label) | ||
valid_attribute?(image) | ||
valid_attribute?(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say you're over-validating here. You should only validate attributes that will cause your site to break if they're absent.
As an example of this, when I searched for "tacos" one of the results had no health labels, which produced an error here.
|
||
root 'recipes#new', as: 'root' | ||
|
||
resources :recipes, only: [:index, :new, :show, :create] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that new
is an appropriate name for the search action, since it's not creating a new recipe. Similarly, I don't think you need the new
or create
resources for recipes
here.
</body> | ||
<footer> | ||
<div id="edamam-badge" data-color="transparent"></div> | ||
</footer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This <footer>
(and all rendering content) should be inside the <body>
tag.
|
||
VCR.use_cassette("recipes") do | ||
proc { | ||
get recipes_path }.must_raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you've identified this as a test case. I'm not sure that I agree with your decision about how to handle this situation - in general, the controller should catch exceptions and pass them on to the user as polite error messages. However, that doesn't invalidate the need to test this case.
it 'raises an error with invalid data' do | ||
VCR.use_cassette("recipe") do | ||
proc { | ||
get recipe_path(owwwww) }.must_raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will raise, but not for the reason you think. The variable owwww
is undefined here, which will produce a NameError
. That's why it's always good to look for a specific error if you can.
VCR.use_cassette("recipes") do | ||
recipes = EdamamApiWrapper.list_recipes("cheese") | ||
|
||
recipes.each do |recipe| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the search term produces 0 results (e.g. lajdflgndflha
)?
describe 'display_recipe' do | ||
it 'returns one recipe' do | ||
VCR.use_cassette("recipes") do | ||
recipe = EdamamApiWrapper.display_recipe("9b5945e03f05acbf9d69625138385408") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you search for a bogus URI?
API Muncher
Congratulations! You're submitting your assignment!
Comprehension Questions
Provide a link to the Trello board you used | https://trello.com/b/tovPieWo/api-tizers