-
Notifications
You must be signed in to change notification settings - Fork 515
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
Idea: Grouping Ingredients #301
Comments
No precise ideas here yet, but as a general suggestion: it'd likely make sense to re-use the same grouping behaviour / programming interface across both ingredients and instructions. Some challenges:
|
I was thinking about this more generally. I think that it could be good if That is, we could capture rating count, rating average, min rating and max rating (all optional) in one
Similarly for ingredients or instructions, we could let that handle the grouping under the hood:
but where
and you could
and
If that was handled as a list of 2 tuples or as some dict, that would be abstracted away from the code that sets it in the scraper |
@weightwatchers-carlanderson interesting - if I interpret your suggestion correctly, would this mean that we could make these changes internally within the scrapers even if the external API for the library remains the same, at least to begin with? In particular I really like your idea about making the |
the interface would be the same but the return types would be different:
That would still break all calling code and unit tests. As many scraper methods use
|
I like it, but I'm hoping you have ideas on how to implement it properly in a way that's coherent. It might be overwhelming if we have users overriding any one of 20 different functions etc. Something that could go along with this is changing the functions that are overridden. Right now we have users write the public functions, |
@bfcarpio perhaps I'm misunderstanding your point and you understand this codebase much better than I but There are couple of things that one could do:
becomes
This will catch scrapers that don't match the right signature. and
|
P.S. rather than P.P.S.
There should be a
|
Sorry, I think I was mixing two different bug plans when I was writing that response. I do like the idea of returning objects (
I think there initially was an implementation where methods didn't need to be defined, but then people started adding in scrapers that didn't work. This was around the time where I found the package so I don't have the most context in this regard. |
@bfcarpio @weightwatchers-carlanderson @vabene1111 pinging you to mention some recent discussion in #622 that is relevant to this feature request about grouping ingredients. The currently-suggested approach there incorporates some ideas I gathered from here - including dataclasses as mentioned by @bfcarpio, and per-field return types suggested by @weightwatchers-carlanderson (that would fit in nicely with our recently-added support for @vabene1111 this is mostly informational for you, but I think that whether or not scraper methods accept parameters could be important for some of the data export functionality - like figuring out which methods to include and how to invoke them. |
awesome thanks. You seem to already have many good ideas regarding this topic. I am facing a similar issue in tandoor (where i am also using recipe-scrapers under the hood). What we have is an algorthm to split a string of ingredients into unit, amount and food, after that we can combine foods of the same unit. What we are currently working on/planning is adding automatic unit conversion so that you can also combine foods of differnt units. One thing that might be hard for this library is that converting a string into unit, amount and food is somewhat error prone and usually requires some human work afterwards, so it might be a bit tricky to go with this approach in recipe-scrapers. |
@vabene1111 agreed: identifying ingredients and units, and then standardizing units is hard. We don't plan to handle those in |
Thanks for the offer, there is currently no open issue as the solution we currently use works pretty well but if you have any ideas on improving it further feel free to open a discussion or issue, thanks :) |
I'm going to list some thoughts here that are based on this PR, but not PR focused. I like the idea of making dataclasses that are
@vabene1111 I just started using Tandoor since you guys are a consumer of ours. I figured it'd be cool to see a project I contribute to in action. Thanks for all your work! |
As of version 14.17.1 (just released), there's support for |
Groups sounds like a great idea.
(I think I am supposed to be getting Ideally it would be returned something like this...
|
Hi guys! I think this would be a great idea. I've had a go at prototyping something that make be generic enough to be useful and I think is in alignment with thoughts further up this thread. The class AbstractScraper:
...
def ingredient_groups(self) -> List[IngredientGroup]:
return [IngredientGroup(ingredients=self.ingredients(), purpose=None)] where The There might also be an opportunity to provide a utility function to help with the grouping the ingredients. Looking at a couple of examples:
The structure of the markup for the ingredients is quite similar. There's a group heading element, followed by some siblings with the ingredients, then another group heading element etc. A helper function along the lines of def group_ingredients(soup, group_heading, group_element):
"""Group ingredients into sublists according the heading
If the recipe doesn't group ingredients, this returns a single IngredientGroup object
with the purpose set to None and the ingredients list populated with all ingredients.
INPUTS
=====
soup : BeautifulSoup
Scraped page html as BeautifulSoup object
group_heading : str
CSS selector for ingredient group heading
group_element : str
CSS selector for ingredient element
OUTPUTS
=====
List[IngredientGroup]
List of IngredientGroup objects.
"""
groupings = {None: []}
current_heading = None
elements = scraper.soup.select(",".join([group_heading, group_element]))
for el in elements:
if el in el.parent.select(group_heading):
# This is the heading
current_heading = normalize_string(el.get_text())
groupings[current_heading] = []
else:
text = normalize_string(el.get_text())
groupings[current_heading].append(text)
return [IngredientGroup(purpose=k, ingredients=v) for k, v in groupings.items() if v != []] This works quite well on some examples.
or
But this isn't perfect. Because the ingredients are extracted from the html markup they might not match list of ingredients returned by |
Hi @strangetom - hope you had a good weekend. This short reply doesn't really do your comment justice, but I think that that sounds fantastic, and I'd be glad to help review -- and with a bit of luck, merge -- your changes to support ingredient groupings. |
thanks for the concept/idea. In my opinion the last "disclaimer" disqualifies this approach. If i cannot rely on the parser picking up all ingredients in the correct amounts than the step ordered ingredients can never be consumed as I would always live in fear of missing something (both wen using the data in scripts but also when archiving). But I dont think the approach is completely flawed. Maybe the scraper could use the ingredients found in the way described above together with some kind of matching algorithm (maybe similar to how I do it in tandoor https://github.com/TandoorRecipes/recipes/blob/develop/vue/src/apps/ImportView/ImportViewStepEditor.vue#L194) to sort the list returned by the normal |
Thanks for the feedback. @vabene1111 , I agree with you that it's not robust enough yet. If Another, simpler, approach would be to use my suggested @jayaddison I would certainly appreciate any help you can offer. I will have a look at opening a draft PR in a few days or so and hopefully we can turn it into something useful. |
I think we can consider this implemented by #781 - it's now available for a number of scrapers. |
@jayaddison mentioned as part of #299 the potential inclusion of grouping ingredients. I remember having a similar conversation about instructions (#230)
If we were to implement this we'd need to change the ingredients methods (or add) so that there's a potential for returning a nested data structure or some other deviation from the current implementation ( akin to #283 ).
Just for brainstorming, should we and how would we?
The text was updated successfully, but these errors were encountered: