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

Add donna hay #1153

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Add donna hay #1153

wants to merge 27 commits into from

Conversation

mlduff
Copy link
Contributor

@mlduff mlduff commented Jun 19, 2024

Resolves #1150

No schema support. Most functions are supported (except for times).

Worked on collaboratively by myself, @heathrampazis , @a1831319 and @Mooree003.

@jayaddison
Copy link
Collaborator

Thanks all! And apologies for taking a while here; I plan to review this within the next 24h or so.

Copy link
Collaborator

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good! I have two requests after reading through the code:

  • Could we try retrieving the recipe title from one of the other elements on the page, and filtering out the pipe (|) and subsequent content from that? I think that would make for more readable recipe titles.
  • To confirm that the ingredient_groups functionality works as expected, could we add another test case for a recipe that involves ingredient groupings?

@a1831319
Copy link
Contributor

  • To confirm that the ingredient_groups functionality works as expected, could we add another test case for a recipe that involves ingredient groupings?

Can do, I'll use https://www.donnahay.com.au/recipes/snacks-and-sides/smoky-eggplant-dip-with-hand-cut-potato-chips as the target if that works?

@jayaddison
Copy link
Collaborator

Sounds good - thanks, @a1831319!

@jayaddison
Copy link
Collaborator

* To confirm that the `ingredient_groups` functionality works as expected, could we add another test case for a recipe that involves ingredient groupings?

Resolved, thank you @a1831319 @mlduff!

* Could we try retrieving the recipe `title` from one of the other elements on the page, and filtering out the pipe (`|`) and subsequent content from that?  I think that would make for more readable recipe titles.

This isn't completely resolved yet - could we use the HTML <title> element, or og:title from a meta tag to retrieve the recipe title instead?

@@ -38,7 +38,7 @@ def site_name(self):
return "Donna Hay"

def title(self):
return self.soup.find("h1", class_="recipe-title__mobile").text
return self.soup.find("title").text.split("|")[0].strip()
Copy link
Collaborator

@jayaddison jayaddison Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return self.soup.find("title").text.split("|")[0].strip()
html_title = self.soup.find("title")
recipe_title, _, _ = html_title.text.partition("|")
return recipe_title.strip()

Edit: call str.partition instead of str.rpartition

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nope.. not quite correct. rpartition would return an empty result when | is not found in the string.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(updated/fixed to use str.partition instead)

Copy link
Collaborator

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you @a1831319 @heathrampazis @mlduff @Mooree003!

Ready to merge once the merge conflict in __init__.py is resolved; the str.partition usage suggestion is optional.

Comment on lines +1 to +2
# mypy: allow-untyped-defs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is generally ready I think - just some merge conflicts to resolve.

There's a small cleanup opportunity here too - after #1174 we don't need these allow-untyped-defs mypy directives, so this can be removed from the file header.

@@ -0,0 +1,62 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note: we've begun checking for a preferred ordering of the JSON key names (not alphabetical; more like priority/review-aid based).

After merging recent changes into your branch, one of the unit tests may begin complaining about the JSON files because of that. There is however a script provided that can automatically fix them -- running python scripts/reorder_json_keys.py should do that for you.

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.

Add www.donnahay.com.au
4 participants