Update the scraper-generator template. Inherit from abc.ABC in AbstractScraper

[jayaddison]

Perhaps we should only include 'core' recipe fields within the auto-generated scrapers. This pull request omits originally omitted a few fields that I'd consider more on the optional side, related to recipes. Inspired by discussion in #663 (cc @bickerdyke).

It's always possible for people to implement the additional fields - and that should happen naturally over time when people decide that they want that information.

This would reduce the work to implement scrapers from scratch, particularly time spent putting together test case data and implementing BeautifulSoup queries.

[jayaddison]

Hmm. Or perhaps we should match the fields to the default-exception-handling field list.

[jayaddison]

Hmm. Or perhaps we should match the fields to the default-exception-handling field list.

Nope, I don't think so after all. A few of those fields are definitely optional (and in fact, ratings is in there too).

[jayaddison]

A related idea: we could make AbstractScraper inherit from ABC (Python's abstract base class), and then require that some of the methods are implemented (by tagging them as @abstractmethod).

As usual, I'm not sure whether it's worthwhile. It'd be stricter - but is that good for developers and the library?

And what are the required elements of a recipe? (ingredients and instructions, certainly - anything else?)

[hhursev]

I'm on board with the idea of inheriting abc.ABC + abstractmethod. It'll be good for developers and the library imo.

The 'core' recipe fields are: host, title, total_time, image, ingredients, instructions.

[hhursev]

In case where site does not have time information for their recipes:

raise RecipeScrapersExceptions(f"{self.host} does not provide time information.")

or sth along these lines to set an example.

[hhursev]

I really like what I see here! Why the CI didn't run 🤔 Is it a glitch or I'm missing something

[jayaddison]

Thanks for the review! Ah, CI didn't run here because the branch changed (originally I had this stacked onto some of the decorator-related field retrieval). Should be fixed when I push a commit for the exception change.

[jayaddison]

Should we consider this a breaking/major version change? (I hadn't thought of this until now, but it could cause breakage for anyone developing private scrapers inherited from AbstractScraper)

[hhursev]

Ah nice thinking .. do not merge into main for now please. Let's create a v15 branch and merge it there instead for the time being.

This would be a major version bump indeed and I'd like us to unite it with other things (removal of the settings module, some scrapers updates due to the health check thing and I probably forget other things we'll introduce too).

Not sure what's the best way to notify current package users. Ideally minor version bump with message during install 🤔 (not sure how this can be done). Simply a message when recipe_scrapers is imported might work too.

[jayaddison]

Ok, the v15 development branch now exists (with unit testing and linting enabled.. as should be visible in this pull request for any subsequent commits).

[jayaddison]

It could be worth figuring out what (if anything) to do about the disallow_untyped_defs=False directives across the codebase as we start on two parallel branches.

I feel like those could cause a bunch of merge conflicts between main and v15 if we're not careful.

(allow-untyped-defs seems much clearer to me than disallow_untyped_defs=False)