Add artworks parser #316
Open
Add artworks parser #316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattseddon
commented
Apr 6, 2025
| ARTWORK_CSS_CLASS = '.iELo6' | ||
| EXTENSION_CSS_CLASS = '.cxzHyb' | ||
| NAME_CSS_CLASS = '.pgNMRc' | ||
| IMG_CSS_CLASS = '.taFZJe' |
There was a problem hiding this comment.
[C] I have these hoisted up here so that they are easy to identify and change.
mattseddon
commented
Apr 6, 2025
| end | ||
|
|
||
| def decode_hex(str) | ||
| str.gsub(/\\x([0-9A-Fa-f]{2})/) { [Regexp.last_match(1)].pack('H2') } |
There was a problem hiding this comment.
[C] There may be a language/built-in library feature that can do this more elegantly but I was unable to find it.
mattseddon
commented
Apr 6, 2025
| describe GoogleArtworksParser do | ||
| subject { described_class.parse } | ||
|
|
||
| fixtures.each do |fixture_name, fixture| |
There was a problem hiding this comment.
[C] I leaned on the relevant tests here.
I'm not too sure about generating tests via a loop. Rather than second-guess myself, I put the PR up. We can discuss whether or not this is standard practice in Ruby.
There was a problem hiding this comment.
No worries. We are setting up our tests this way.
mattseddon
force-pushed
the
seddon/add-google-artworks-parser
branch
from
c07f258 to
a9a6969
Compare
mattseddon
force-pushed
the
seddon/add-google-artworks-parser
branch
from
a9a6969 to
dcdaf5f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my submission for the coding challenge.
Approach
I have used your
nokolexborparser and CSS classes to identify the artworks section within the HTML. As some images appear to be lazy loaded, I first search for them within the document (using script tags containing lazy loaded images based on regex) and generate a hash of{id => img}. I then parse the artworks out of the HTML and use the hash contents as a priority before falling back to thedata-srcattribute of each img. The rest of the required information seemed readily available in the HTML.I took this approach instead of using a headless browser to load the images (i.e. run the embedded JavaScript) because I feel it is more efficient and it avoids adding another external dependency to the project. We would need a benchmark for the first assumption if this was production code (instead of a coding challenge).
The main drawback I see to this approach is that it may be brittle. It has been a long time since I have done any scraping. I am not sure how often CSS classes will change on these page types and/or how often the underlying JavaScript gets updated. Changes to either will break the parser. I would definitely want a test directly against Google running often to confirm that everything is still working.
Things that are missing/next steps:
Additional info
This is the first time (ever) that I've written Ruby, I used
rbenvandrubocopfor the project. I took on board most of the "suggestions" fromrubocopdue to my lack of experience with the language. I contemplated addingsorbetbut ultimately decided against it because I could not see any trace innokolexbor.I have left some comments inline.
Thanks for taking the time to review.