-
Notifications
You must be signed in to change notification settings - Fork 25
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
Tiger level challenge #24
base: master
Are you sure you want to change the base?
Conversation
…ersion of movie_json.rb
…mmer2012 to change commit message
class Api | ||
|
||
APIKEY="4t6456xa33z8qhcqyuqgnkjh" | ||
APIKEY=API_KEY |
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 the moving the key into a git-ignored file. But, you can just have that set your variable... in api_key.rb
have it set APIKEY directly. This line ends up looking odd (and future-you won't remember what this is)
Great job on the specs! And I really love the thought of security with the gitignore. Let me know if you have any questions on my comments :) |
…d add to movie list methods. fix specs
… and movie_rating_slope methods
Tried to fix the things you suggested, let me know if this looks better! Thanks |
year: struct.year, | ||
score: struct.ratings["critics_score"] | ||
) | ||
struct.id.nil? ? NullObject.new : store_movie(struct) |
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.
Looks great!!
Only comment: rather than "store_movie", recommend "build_movie". Store tends to imply persistence.
Looks greast! Significant improvement in readability. yay! |
👍 🎉 |
I replaced the api_key with mine from a .gitignore file, FYI. Also, 1 question, in movie_json.rb #movie_search_by_title is complicated. Is this best fixed with more, simpler, methods? Thanks!