openzim / python-scraperlib

Collection of Python code to re-use across Python-based scrapers
GNU General Public License v3.0
18 stars 16 forks source link

Type hints of __init__ in StaticItem are not correct #109

Closed benoit74 closed 4 months ago

benoit74 commented 11 months ago

The __init__ function of StaticItem is inherited from Item.

The type hint is incorrect in Item because it miss at least the callback attribute.

The type hint is even more incorrect in StaticItem because it misses all additional attributes from this class.

In addition to these two issues, I have a newbie question. Can't we write the __init__ in a less generic way, i.e. with all attributes specified instead of **kwargs ? I imagine this would imply more maintenance every time libzim is modified, but at the same time would help a lot to discover all possible attributes + have correct type hints for every of them. But I might probably miss something else.

rgaudin commented 11 months ago

Indeed hints are incorrect.

As to make it non-generic, no, that's the point of our Item: you send whatever you want and it's stored in the object so you can reuse it in the methods. This Item is a helper, one can reuse libzim's or subclass it. I think fixing the hints for generic usage is probably the first step.

But we can discuss making it less generic in a future release. We now have a lot of experience with it and know what real usage is. scraperlib should serve the 80% use cases

kelson42 commented 6 months ago

What is missing here to complete? Any chance to close this PR?

benoit74 commented 4 months ago

As seen recently in kolibri, there is no callback in Item, the callback is implemented in add_item. So the only mistake is around the typing of kwargs (we need to pass only the type of the arg values, Python takes care of the fact that it is a dict by itself)

benoit74 commented 4 months ago

In fact there has been a callback at some point: https://github.com/openzim/python-scraperlib/commit/4d123f94c64154b514c4dbacc3d45403096a52c3 But it has been quickly removed few months later: https://github.com/openzim/python-scraperlib/commit/c47333f469910d712aedb5b5e0f2ecf97b5fa44a