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

Added owen-han.com #599

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Added owen-han.com #599

merged 3 commits into from
Sep 12, 2022

Conversation

ephemer4l
Copy link
Contributor

This is my first time contributing. Please let me know if there's any improvement to be made.

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! Thanks @ephemer4l

@jayaddison
Copy link
Collaborator

Ah, yep - I was planning to look into the schema.org / title retrieval problem too. I'll take a look at that now.

@jayaddison
Copy link
Collaborator

jayaddison commented Sep 12, 2022

Ok, yep - the reason the title field can't be access via self.schema:

Although the site provides schema.org metadata in ld+json format, the relevant title field is within some markup that has @type:Article -- and that's not included in the list of schema names that we currently parse from.

Adding Article to that list of schema names doesn't seem to break any unit tests. I don't think we need to (or should) do that within this pull request - I think this scraper is ready as-is. But I'll do a bit more investigation and perhaps we can enable that soon, allowing use of self.schema.title() in this scraper (and potentially others, too).

@jayaddison jayaddison merged commit 3a57112 into hhursev:main Sep 12, 2022
@jayaddison
Copy link
Collaborator

This scraper is now available on PyPi in recipe-scrapers v14.14.0 - thanks again, @ephemer4l!

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.

2 participants