Skip to content
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

Handle Recipe Times as dicts and lists #2764

Conversation

Kuchenpirat
Copy link
Collaborator

@Kuchenpirat Kuchenpirat commented Nov 27, 2023

What type of PR is this?

  • bug/cleanup/feature (am not sure how to classify)

What this PR does / why we need it:

Recipe times do come sometimes in form of a dict. This parses the dict if it contains an minValue and returns this. This mimiques the recipe scraper which impelemts this here.

Which issue(s) this PR fixes:

fixes #2753
fixes #2732

Special notes for your reviewer:

I decided to return None instead of the TypeError if the minValue entry is not present. This will continue the scraping and allow the recipe to be imported.

We might think of doing the same for the default case. I think the total, prep and cook times are non essential to an recipe and important times usually are stated within the recipe. If we return None istead of raising an type error this would just remove those fields and will create the recipe without them.

Testing

Manual testing, can write a test case for this if desired.

@michael-genson
Copy link
Collaborator

michael-genson commented Nov 27, 2023

Would there be any harm of just grabbing a the first value instead of returning None? I feel like it would be most likely to grab something useful.

Also, as long as we're in here, any chance you could tackle #2732 as well? Should be pretty much the same thing, just grab the first element if the list isn't empty

@Kuchenpirat
Copy link
Collaborator Author

Kuchenpirat commented Nov 27, 2023

Am not to sure about just returning the first element in the dict as we cannot be sure what that would be. First value would be "Duration" in this example and maxValue is 1485 min. So 2/3 have nothing to do with the actual time.
But i am happy to be convinced otherwise.

Will implement the array 👍 Tho there are obviously the same risks as above. But i think the chance is lesser to find garbage data in there.

@michael-genson
Copy link
Collaborator

So 2/3 have nothing to do with the actual time

Makes sense to me!

@Kuchenpirat
Copy link
Collaborator Author

Kuchenpirat commented Nov 27, 2023

#2732 seems to just run into the next error with image scraping because it is missing the domain.

while i am in there, any thoughts on this? regarding turning the default case to none?

We might think of doing the same for the default case. I think the total, prep and cook times are non essential to an recipe and important times usually are stated within the recipe. If we return None istead of raising an type error this would just remove those fields and will create the recipe without them.

@michael-genson
Copy link
Collaborator

I think that makes sense, as you said it's nonessential. Maybe put a warning log in there just in case someone wants to raise an issue for it

from mealie.core.root_logger import get_logger

logger = get_logger(__name__)
logger.warning(...)

@michael-genson
Copy link
Collaborator

Otherwise PR LGTM!

mealie/services/scraper/cleaner.py Outdated Show resolved Hide resolved
@Kuchenpirat Kuchenpirat changed the title Handle Recipe Times as dicts Handle Recipe Times as dicts and lists Nov 27, 2023
@Kuchenpirat Kuchenpirat requested a review from hay-kot November 28, 2023 13:08
@hay-kot hay-kot enabled auto-merge (squash) December 3, 2023 05:10
@hay-kot hay-kot merged commit f32444b into mealie-recipes:mealie-next Dec 3, 2023
6 checks passed
@Kuchenpirat Kuchenpirat deleted the handle-recipe-times-containing-dict branch December 3, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SCRAPER] - YOUR TITLE [SCRAPER] - https://www.bhg.com.au doesn't work
3 participants