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

Deprecate scrapy.item.BaseItem #4534

Merged
merged 12 commits into from May 15, 2020
Merged

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented May 4, 2020

@elacuesta elacuesta added this to the v2.2 milestone May 4, 2020
@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #4534 into master will increase coverage by 0.02%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #4534      +/-   ##
==========================================
+ Coverage   84.55%   84.57%   +0.02%     
==========================================
  Files         164      164              
  Lines        9923     9932       +9     
  Branches     1475     1477       +2     
==========================================
+ Hits         8390     8400      +10     
  Misses       1266     1266              
+ Partials      267      266       -1     
Impacted Files Coverage Δ
scrapy/spiders/feed.py 66.15% <ø> (ø)
scrapy/commands/parse.py 20.21% <50.00%> (ø)
scrapy/contracts/default.py 84.31% <50.00%> (ø)
scrapy/core/scraper.py 87.97% <50.00%> (ø)
scrapy/exporters.py 100.00% <100.00%> (ø)
scrapy/item.py 98.73% <100.00%> (+0.16%) ⬆️
scrapy/shell.py 68.21% <100.00%> (ø)
scrapy/utils/misc.py 97.27% <100.00%> (ø)
scrapy/utils/serialize.py 93.54% <100.00%> (ø)
scrapy/utils/trackref.py 85.71% <0.00%> (+2.85%) ⬆️

tests/test_item.py Show resolved Hide resolved
scrapy/item.py Show resolved Hide resolved
@Gallaecio
Copy link
Member

Gallaecio commented May 5, 2020

Don’t forget items.rst.

@elacuesta
Copy link
Member Author

elacuesta commented May 5, 2020

Don’t forget items.rst.

I left it there for now because we technically haven't removed it yet, but the autoclass directive will import the docstring which now warns about it being deprecated.

(Sorry, I meant to click "quote reply" but I ended up editing the original comment 😅 )

@Gallaecio
Copy link
Member

Gallaecio commented May 5, 2020

My take is that the documentation should not mention deprecated stuff outside news.rst. Someone reading the documentation for the first time should not need to bother reading about deprecated content.

@elacuesta
Copy link
Member Author

elacuesta commented May 6, 2020

True, and to be fair it's not the first time we discuss this, my bad

@elacuesta elacuesta closed this May 7, 2020
@elacuesta elacuesta reopened this May 7, 2020
scrapy/item.py Show resolved Hide resolved
scrapy/item.py Show resolved Hide resolved
@kmike
Copy link
Member

kmike commented May 8, 2020

Looks good, thanks @elacuesta! I left a small comment about a docstring, but I think we're good to merge otherwise. 👍

@elacuesta
Copy link
Member Author

elacuesta commented May 8, 2020

Updated, thanks @kmike


If you need instances of a custom class to be considered items by Scrapy,
you must inherit from either :class:`BaseItem` or :class:`dict`.
class _BaseItemMeta(ABCMeta):
Copy link
Member

@dangra dangra May 11, 2020

Choose a reason for hiding this comment

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

It looks like we are trying to reinvent create_deprecated_class util from

def create_deprecated_class(
name,
new_class,
clsdict=None,
warn_category=ScrapyDeprecationWarning,
warn_once=True,
old_class_path=None,
new_class_path=None,
subclass_warn_message="{cls} inherits from deprecated class {old}, please inherit from {new}.",
instance_warn_message="{cls} is deprecated, instantiate {new} instead."
):
"""
Return a "deprecated" class that causes its subclasses to issue a warning.
Subclasses of ``new_class`` are considered subclasses of this class.
It also warns when the deprecated class is instantiated, but do not when
its subclasses are instantiated.
It can be used to rename a base class in a library. For example, if we
have
class OldName(SomeClass):
# ...
and we want to rename it to NewName, we can do the following::
class NewName(SomeClass):
# ...
OldName = create_deprecated_class('OldName', NewName)
Then, if user class inherits from OldName, warning is issued. Also, if
some code uses ``issubclass(sub, OldName)`` or ``isinstance(sub(), OldName)``
checks they'll still return True if sub is a subclass of NewName instead of
OldName.
"""
class DeprecatedClass(new_class.__class__):
deprecated_class = None
warned_on_subclass = False
def __new__(metacls, name, bases, clsdict_):
cls = super(DeprecatedClass, metacls).__new__(metacls, name, bases, clsdict_)
if metacls.deprecated_class is None:
metacls.deprecated_class = cls
return cls
def __init__(cls, name, bases, clsdict_):
meta = cls.__class__
old = meta.deprecated_class
if old in bases and not (warn_once and meta.warned_on_subclass):
meta.warned_on_subclass = True
msg = subclass_warn_message.format(cls=_clspath(cls),
old=_clspath(old, old_class_path),
new=_clspath(new_class, new_class_path))
if warn_once:
msg += ' (warning only on first subclass, there may be others)'
warnings.warn(msg, warn_category, stacklevel=2)
super(DeprecatedClass, cls).__init__(name, bases, clsdict_)
# see https://www.python.org/dev/peps/pep-3119/#overloading-isinstance-and-issubclass
# and https://docs.python.org/reference/datamodel.html#customizing-instance-and-subclass-checks
# for implementation details
def __instancecheck__(cls, inst):
return any(cls.__subclasscheck__(c)
for c in {type(inst), inst.__class__})
def __subclasscheck__(cls, sub):
if cls is not DeprecatedClass.deprecated_class:
# we should do the magic only if second `issubclass` argument
# is the deprecated class itself - subclasses of the
# deprecated class should not use custom `__subclasscheck__`
# method.
return super(DeprecatedClass, cls).__subclasscheck__(sub)
if not inspect.isclass(sub):
raise TypeError("issubclass() arg 1 must be a class")
mro = getattr(sub, '__mro__', ())
return any(c in {cls, new_class} for c in mro)
def __call__(cls, *args, **kwargs):
old = DeprecatedClass.deprecated_class
if cls is old:
msg = instance_warn_message.format(cls=_clspath(cls, old_class_path),
new=_clspath(new_class, new_class_path))
warnings.warn(msg, warn_category, stacklevel=2)
return super(DeprecatedClass, cls).__call__(*args, **kwargs)
deprecated_cls = DeprecatedClass(name, (new_class,), clsdict or {})
try:
frm = inspect.stack()[1]
parent_module = inspect.getmodule(frm[0])
if parent_module is not None:
deprecated_cls.__module__ = parent_module.__name__
except Exception as e:
# Sometimes inspect.stack() fails (e.g. when the first import of
# deprecated class is in jinja2 template). __module__ attribute is not
# important enough to raise an exception as users may be unable
# to fix inspect.stack() errors.
warnings.warn("Error detecting parent module: %r" % e)
return deprecated_cls

Copy link
Member Author

@elacuesta elacuesta May 11, 2020

Choose a reason for hiding this comment

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

I looked (briefly) at that function before doing these changes, for some reason it didn't appear appropriate for this case. But I can't actually remember what that reason was, so I'll check again 👍

Copy link
Member Author

@elacuesta elacuesta May 11, 2020

Choose a reason for hiding this comment

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

Right, I tried this and remembered why I didn't use it. If I understand correctly, what you suggest is to do BaseItem = create_deprecated_class('BaseItem', Item). The problem I see is that we don't want the deprecated BaseItem to have the functionality of Item, i.e. the field capabilities provided by the ItemMeta meta class:

In [1]: from scrapy.item import Item, Field 
   ...: from scrapy.utils.deprecate import create_deprecated_class 
   ...:  
   ...: BaseItem = create_deprecated_class('BaseItem', Item, old_class_path="scrapy.item.BaseItem")  
   ...:  
   ...: class TestItem(BaseItem): 
   ...:     foo = Field() 
   ...:  
   ...: print(TestItem(foo="bar"))                                                                                                                                                                                                            
/.../scrapy/venv-scrapy/bin/ipython:6: ScrapyDeprecationWarning: __main__.TestItem inherits from deprecated class scrapy.item.BaseItem, please inherit from scrapy.item.Item. (warning only on first subclass, there may be others)
  
{'foo': 'bar'}

@dangra Please correct me if I'm wrong. Thanks!

Copy link
Member Author

@elacuesta elacuesta May 14, 2020

Choose a reason for hiding this comment

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

Ping @dangra 😄

scrapy/item.py Outdated Show resolved Hide resolved
kmike
kmike approved these changes May 12, 2020
Copy link
Member

@kmike kmike left a comment

Looks good to me, thanks @elacuesta!

@kmike
Copy link
Member

kmike commented May 15, 2020

Let's merge it :) @dangra - if you think something needs to be changed, please let us know, and we'll send a PR with a fix.

@kmike kmike merged commit 14612fc into scrapy:master May 15, 2020
2 checks passed
@kmike
Copy link
Member

kmike commented May 15, 2020

Thanks @elacuesta!

@elacuesta elacuesta deleted the deprecate-baseitem branch May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants