Feature/scraper #1
Conversation
Also added minitest pride because colors
Too brittle any other way
Combined the assertions in one spec to speed up tests. Will separate later when not running suite over and over again.
| @@ -0,0 +1,8 @@ | |||
| # A sample Gemfile | |||
|
looks good to me 👍 although I am not too experienced with all of this so Clemens should also go over it |
| @award_title = get_award_title | ||
| @country = get_country | ||
| @description = get_description | ||
| end |
There was a problem hiding this comment.
This seems somewhat redundant, especially @award = won?, because won? already assigns this instance variable.
I'd suggest renaming methods like get_title to title, let them cache their result in the respective instance variable and make them public. This way you also don't need the attr_readers.
Then your parse method could look like this:
def parse
%i(title year organization details_link category award_won award_title country description).each(method(:public_send))
endThis way you could also choose to calculate all attributes in advance – by calling parse – or to lazy evaluate the attributes – by just calling the getters when you need them.
There was a problem hiding this comment.
I don't like this. But i did something earlier with Array.zip to do a to_json type of thing.
After using go, I am much more likely to do the title instead of get_title because it is obvious. So you're right, more memoized method that grab the value on first access would be reasonable. I will make that change.
The thing with won? is that i wanted a won? method. I guess I am in a phase where I don't like to do anything surprising... this really doesn't make any sense. I used to always do this. Maybe working with other languages can make your better language weird too. I can talk about this in India...
There was a problem hiding this comment.
Fixed this in latest commit 1b2721a
I just removed parse instead of your suggestion above
|
I made updates based on @clemenshelm feedback. |
| def [](_) "" end | ||
| def text() "" end | ||
| def method_missing(*_) NullElement.new end | ||
| end |
There was a problem hiding this comment.
This is pretty sweet! One little remark about quotes: Do we choose option B from the style guide? https://github.com/bbatsov/ruby-style-guide#consistent-string-literals
Would be really ok for me.
There was a problem hiding this comment.
I generally use single quotes because I heard once that it is faster because it doesn't need to try to worry about string interpolation. As you can see, it isn't a standard of mine but I will be more consistent in the future, choosing single over double unless interpolation is happening.
There was a problem hiding this comment.
The speed advantage is very little. But I'm good with every convention. So do you wanna go for option A or option B?
|
🐨 Awesome! Looking really good now! |
| if details && (slides = @details.at_css(".slides_container")) | ||
| slides.css('img').to_a | ||
| .compact | ||
| .map { |img| "%s%s" % [@base_url, img['src']]} |
There was a problem hiding this comment.
Is this how you usually write method chains?
I usually prefer to write it like
slides
.css('img').to_a
.compact
.map { |img| "%s%s" % [@base_url, img['src']] }Which style do you like better?
There was a problem hiding this comment.
Oh, and there's a space missing before the closing curly brace.
|
Ok, let's merge this bitch |
|
ok? @clemenshelm |
|
I can't. Can you merge the master and resolve the conflicts? |
| if you haven't bundled | ||
| `bundle install` | ||
| then | ||
| `ruby spec/all.rb` |
There was a problem hiding this comment.
There's still the old spec file mentioned.
|
yes |
Scraper works