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 wrong checks on subclassing of deprecated classes #584

merged 1 commit into from Feb 5, 2014


Copy link

@dangra dangra commented Feb 5, 2014

closes #581

Copy link

@nramirezuy nramirezuy commented Feb 5, 2014

Those tests are great +1

Copy link
Member Author

@dangra dangra commented Feb 5, 2014

/cc @kmike as you wrote the initial version of this util

candidates = {cls, new_class}
candidates = {cls}
if cls is DeprecatedClass.deprecated_class:

This comment has been minimized.


kmike Feb 5, 2014

I think it'd be more clear what's going on if we move this check to the top:

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__', ())
    candidates = {cls, new_class}
    return any(c in candidates for c in mro)

This comment has been minimized.


dangra Feb 5, 2014
Author Member

+1 I amended the commit and will merge

Copy link

@kmike kmike commented Feb 5, 2014

Good catch @nramirezuy and good fix @dangra - these __subclasscheck__ methods can drive anyone nuts.

dangra added a commit that referenced this pull request Feb 5, 2014
Fix wrong checks on subclassing of deprecated classes
@dangra dangra merged commit 3d4fe60 into scrapy:master Feb 5, 2014
@dangra dangra deleted the dangra:581-deprecated-subclass-fix branch Feb 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants