Opened 3 years ago

Closed 2 years ago

#17447 closed Cleanup/optimization (worksforme)

Problematic logical expressions for types.ClassType checks in django.utils.dictconfig

Reported by: zimmermann Owned by: nobody
Component: Python 3 Version: master
Severity: Normal Keywords: types ClassType logic expression hasattr
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

BaseConfigurator.configure_custom and DictConfigurator.configure_handler both contain the following code:

if not hasattr(c, '__call__') and hasattr(types, 'ClassType') and type(c) != types.ClassType:
    c = self.resolve(c)

The problem is that this logical expression always fails if the types module doesn't have the ClassType attribute (I encountered this when working with the django-3k fork)

The expression could be written like this:

if not hasattr(c, '__call__') and (not hasattr(types, 'ClassType') or type(c) != types.ClassType):
    c = self.resolve(c)

But the hasattr call could also be completely dropped (In python 2.x types.ClassType is always there and 2to3 converts it to type):

if not hasattr(c, '__call__') and type(c) != types.ClassType:
    c = self.resolve(c)

Attachments (1)

django.utils.dictconfig.diff (1005 bytes) - added by zimmermann 3 years ago.

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by zimmermann

comment:1 Changed 3 years ago by aaugustin

  • Component changed from Uncategorized to Core (Other)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Django's dictconfig is a literal copy of the one in Python 2.7, included verbatim to ensure compatibility across Python versions.

Has this issue been reported and/or fixed in Python's native dictconfig?

If there's a problem with the version bundled with Django, the preferred fix is to update to a new verbatim copy, not to patch Django's version.

comment:2 Changed 3 years ago by lrekucki

  • Triage Stage changed from Unreviewed to Accepted

It a non issue for Python, beacause this code work in Python 2.x, and Python 3.2+ just uses if not callable(c). One way to fix this, without touching the verbatim copy would be to alter 2to3, to fix the check.

I may be missing something, but isn't this check exactly equal to what callable() does? If yes, I'm in favor of changing this.

PS. I wonder what's the story behind this subtle diffrence in what callable() does:

2.x: http://hg.python.org/cpython/file/7c263bfc92f5/Objects/object.c#l1650

3.2: http://hg.python.org/cpython/file/cd748bab3cdd/Objects/object.c#l1179

Version 0, edited 3 years ago by lrekucki (next)

comment:3 Changed 3 years ago by lrekucki

  • Patch needs improvement set

comment:4 Changed 2 years ago by aaugustin

  • Component changed from Core (Other) to Utilities

comment:5 Changed 2 years ago by aaugustin

  • Component changed from Utilities to Python 3
  • Resolution set to worksforme
  • Status changed from new to closed

This bug was filed against an old experimental port to Python 3 that was abandonned.

Django now attemps to import dictConfig from logging.config, and falls back to importing it from django.utils.dictconfig only on Python 2.6.

Note: See TracTickets for help on using tickets.
Back to Top