Add felix.kitchen #1098
Add felix.kitchen #1098
Conversation
mlduff
force-pushed
the
add-felix.kitchen
branch
from
b8f3667
to
e9c1441
Compare
|
Thanks for the review, comments should be addressed now @jayaddison |
recipe_scrapers/felixkitchen.py
Outdated
| def total_time(self): | ||
| return None |
There was a problem hiding this comment.
We may want to emit a Python warning here, if we know that we're never going to find results for this field on this website.
There was a problem hiding this comment.
@jayaddison I've been following #1067, I'm not 100% sure what the verdict on the way you want this implemented is?
There was a problem hiding this comment.
I'm not certain either; whatever approach we choose, I want it to be simple (no more than a few lines), and minimally disruptive, because I think it's somewhat experimental but potentially useful information.
There was a problem hiding this comment.
@jayaddison I've now updated this with my attempt at an approach to warnings that allows for the warning to be silenced during testing.
Co-authored-by: Joey <7505194+jknndy@users.noreply.github.com>
recipe_scrapers/_utils.py
Outdated
| @@ -274,3 +281,14 @@ def change_keys(obj, convert): | |||
| def get_url_slug(url): | |||
| path = url_path_to_dict(url).get("path") | |||
| return path.split("/")[-1] | |||
|
|
|||
|
|
|||
| def unsupported_field_warning(host, field): | |||
There was a problem hiding this comment.
Recommend following the same approach as #1077 for this; only implement for the affected scraper, not as a _utils` module feature yet.
It probably makes sense to share a common code path for these kind of warnings at some point, but let's begin simple.
| return normalize_string(found.get("content")) | ||
|
|
||
| def total_time(self): | ||
| field_not_provided_by_website_warning(self.host(), "total_time") |
There was a problem hiding this comment.
Ok, yep - this is getting closer to a good interface I think.
I'd be tempted to suggest that in fact, this should be implemented as an exception. So all that a new developer needs to know is: raise the correct type of exception.
def total_time(self);
raise FieldNotProvidedByWebsiteException()...and then we'd have plugin -- enabled by default -- to emit a warning when that exception is raised.
That way, if some downstream consumer really wants to handle cases where the website doesn't ever provide data for a field, they can do so.
(that's not possible -- or not easily possible -- using warnings alone, because they don't affect the return value/behaviour of the method)
There was a problem hiding this comment.
I'm going to merge this as-is, but I think we can and should refactor this in the nearish future.
This PR adds support for felix.kitchen, using https://felix.kitchen/2024/04/07/piccata-milanese-schnitzel-parmesan-safran-risotto-petersilie/ as the test recipe.
Couldn't find
total_timeon any recipe on the website, so have this returningNone.Resolves #1063