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

Fix additional details extraction in ATK scraper #1320

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

Conversation

jknndy
Copy link
Collaborator

@jknndy jknndy commented Oct 22, 2024

Implements the fix recommended by @sudobash1 in #1317 and pulls new test data to represent current site structure/information.

Also, pulls new test data and updates tests for two related coverages, cookscountry & cooksillustrated.

Resolves #1317

@jknndy jknndy marked this pull request as ready for review October 22, 2024 21:03
@jayaddison
Copy link
Collaborator

Generally these changes are OK with me; the two things I'd like to figure out before merge are:

  • Should we retain backwards-compatibility with the previous format?
  • What to do about the lengthy description test data.

@jknndy
Copy link
Collaborator Author

jknndy commented Oct 23, 2024

  • Should we retain backwards-compatibility with the previous format?

Normally, I believe backwards compatibility should be a priority, as long as it doesn't degrade performance. However, since it appears all the existing recipes have already been migrated to this new format (with all three test cases requiring updates to pass), it might not be necessary in this case.

@smilerz
Copy link
Contributor

smilerz commented Nov 1, 2024

  • Should we retain backwards-compatibility with the previous format?

Normally, I believe backwards compatibility should be a priority, as long as it doesn't degrade performance. However, since it appears all the existing recipes have already been migrated to this new format (with all three test cases requiring updates to pass), it might not be necessary in this case.

While developing the ATK scraper for v15 this was the initial format and I had to change it at least once during development. It might make sense to provide both until there is some confidence that it is stable. Though not sure how to discover that? Some stdout during tests that the legacy format was detected?

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.

[Americas Test Kitchen] - Scraper broken
5 participants