-
Notifications
You must be signed in to change notification settings - Fork 525
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
add americastestkitchen.com #1060
Conversation
add tests for ATK scraper
tests/test_data/americastestkitchen.com/americastestkitchen.json
Outdated
Show resolved
Hide resolved
added `ingredient_group()``
I have three questions/requests for feedback on the latest commit. Is there a way to avoid duplicating the ingredient formatting code between ingredients() and ingredient_groups()? And finally - in ingredients, sometimes they preface the post_text with ", ...." and sometimes they don't. Is there an elegant way to preface the post text with a space if there isn't some sort of delimiter? |
One more question: ATK also has a format for There are 5 instructions groups with no names assigned, but can be eyeballed to include the above 3 assembly steps and 2 intermediate steps on the actual baking. I'm not sure if it's worth updating the |
I'm not too familiar with that feature to be honest - but I'll investigate that soon and then will let you know (unless someone else gets to that before me).
Yes, for this there is a helper utility - I'd recommend referring to the
Hrm, that's annoying. Maybe we could trim the left of the |
I looked at the |
Interesting! We don't support groups of instructions at the moment - only groups of ingredients. However, both do make sense in recipes, and it's good to have found an example of that -- and to note the possibility for them to be linked in some cases. I'm thinking about what to do here, too. |
Oh, thanks for the clarification - I misread what |
That's OK, they are similar names! :) |
Of course; I forgot that we're using JSON input here. No, I think you've got it - the grouping utils are intended for use with HTML, and they use CSS-based selector queries. I'll try experimenting with the scraper code here to explore other possibilities. |
Thanks - I appreciate the collaboration. |
You're welcome - thanks for the pull request. I'll provide some more review comments within the next day or two. |
I saw the tests failed - my mistake, I forgot to update expected values after stripping the leading zeros from commas. |
Co-authored-by: James Addison <55152140+jayaddison@users.noreply.github.com>
updated tests to match
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.
@smilerz this has been included and released in |
It doesn't look like it's in |
Ah, oops - yep, my mistake (that notification was copy-pasted; I forgot about the different circumstance here :/ ). Thanks @smilerz. |
Co-authored-by: James Addison <james@reciperadar.com>
This reverts commit cff269f.
ATK is behind a paywall - so this only makes sense to add as part of v15