-
Notifications
You must be signed in to change notification settings - Fork 562
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
Improve scraping of spisbedre.dk #1489
Improve scraping of spisbedre.dk #1489
Conversation
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 pretty good! Thanks for the contribution, just a few small suggestions. Let me know what you think.
servings = int(self.recipe_json.get("serving_size")) | ||
yield_type = ( | ||
"item" if self.recipe_json.get("serving_size_unit_id") != 1 else "serving" | ||
) | ||
return f"{servings} {yield_type}{'s' if servings != 1 else ''}" |
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.
servings = int(self.recipe_json.get("serving_size")) | |
yield_type = ( | |
"item" if self.recipe_json.get("serving_size_unit_id") != 1 else "serving" | |
) | |
return f"{servings} {yield_type}{'s' if servings != 1 else ''}" | |
return self.schema.yields() |
We can gather the yield info directly from the json without any custom logic!
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.
That's only partly true as spisbedre.dk only renders a numeric value for the recipeYield tag, which doesn't indicate if the yield is per person or a number of items.
From the get_yields function in _utils.py I gathered that yield should reflect either servings or items, I hope this is a correct assumption.
So, to get the exact indication, it's necessary to fetch the information from the JSON structure which isn't in the JSON-LD format hence the additional parsing.
The third test case (mentioned below) is a recipe for buns, and the yield is number of buns - I hope this makes sense.
"Fyld marengsen i en sprøjtepose, og sprøjt den ud i pæne toppe oven på hver bolle. Brænd marengsen med en lille gasbrænder, så den får gyldne kanter. Servér fastelavnsbollerne straks med en god skefuld lemoncurd på toppen." | ||
], | ||
"category": "Desserter, Kager, Fastelavnsboller", | ||
"yields": "12 items", |
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.
"yields": "12 items", | |
"yields": "12 servings", |
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.
Please see the comment above, regarding the yield formatting.
lgtm, thanks for the explanations |
Improve scraping by including nutrients, and equipment from the recipes.
Formatting of yields and ingredient units are also aligned with the website rendering.
Closes #1488