[Django] #28906: Reduce calls to bool()

14 views
Skip to first unread message

Django

unread,
Dec 7, 2017, 7:56:27 AM12/7/17
to django-...@googlegroups.com
#28906: Reduce calls to bool()
------------------------------------------------+------------------------
Reporter: Дилян Палаузов | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Compared to the attached patch, these changes might be more controversial:
{{{
diff --git a/django/contrib/gis/db/backends/spatialite/features.py
b/django/contrib/gis/db/backends/spatialite/features
--- a/django/contrib/gis/db/backends/spatialite/features.py
+++ b/django/contrib/gis/db/backends/spatialite/features.py
@@ -10,4 +10,4 @@ class DatabaseFeatures(BaseSpatialFeatures,
SQLiteDatabaseFeatures):

@cached_property
def supports_area_geodetic(self):
- return bool(self.connection.ops.lwgeom_version())
+ return self.connection.ops.lwgeom_version()
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -51,7 +51,7 @@ class SQLCompiler:
order_by = self.get_order_by()
self.where, self.having = self.query.where.split_having()
extra_select = self.get_extra_select(order_by, self.select)
- self.has_extra_select = bool(extra_select)
+ self.has_extra_select = extra_select
group_by = self.get_group_by(self.select + extra_select,
order_by)
return extra_select, order_by, group_by
@@ -1216,11 +1216,10 @@ class SQLInsertCompiler(SQLCompiler):
insert_statement =
self.connection.ops.insert_statement(on_conflict=self.query.on_conflict)
result = ['%s %s' % (insert_statement, qn(opts.db_table))]

- has_fields = bool(self.query.fields)
- fields = self.query.fields if has_fields else [opts.pk]
+ fields = self.query.fields or [opts.pk]
result.append('(%s)' % ', '.join(qn(f.column) for f in fields))

- if has_fields:
+ if self.query.fields:
value_rows = [
[self.prepare_value(field, self.pre_save_val(field, obj))
for field in fields]
for obj in self.query.objs
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28906>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Dec 7, 2017, 7:57:16 AM12/7/17
to django-...@googlegroups.com
#28906: Reduce calls to bool()
-------------------------------------+-------------------------------------

Reporter: Дилян Палаузов | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Дилян Палаузов):

* Attachment "less_bool.patch" added.

Django

unread,
Dec 7, 2017, 10:34:06 AM12/7/17
to django-...@googlegroups.com
#28906: Reduce calls to bool()
-------------------------------------+-------------------------------------

Reporter: Дилян Палаузов | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Dufresne):

>{{{


> @cached_property
> def supports_area_geodetic(self):
>- return bool(self.connection.ops.lwgeom_version())
>+ return self.connection.ops.lwgeom_version()
> }}}


>{{{


>- self.has_extra_select = bool(extra_select)
>+ self.has_extra_select = extra_select
>}}}

For methods and members, I think the type might be considered part of the
interface and should perhaps stay.

Reducing `bool()` in expressions seems OK to me though.

--
Ticket URL: <https://code.djangoproject.com/ticket/28906#comment:1>

Django

unread,
Dec 7, 2017, 3:04:07 PM12/7/17
to django-...@googlegroups.com
#28906: Reduce calls to bool()
-------------------------------------+-------------------------------------

Reporter: Дилян Палаузов | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

* component: Uncategorized => Core (Other)
* stage: Unreviewed => Ready for checkin


Comment:

[https://github.com/django/django/pull/9446 PR]. I updated the patch as
per Jon's comments which I also agree with.

--
Ticket URL: <https://code.djangoproject.com/ticket/28906#comment:2>

Django

unread,
Dec 7, 2017, 5:13:26 PM12/7/17
to django-...@googlegroups.com
#28906: Reduce calls to bool()
-------------------------------------+-------------------------------------

Reporter: Дилян Палаузов | Owner: nobody
Type: | Status: closed

Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by GitHub <noreply@…>):

* status: new => closed
* resolution: => fixed


Comment:

In [changeset:"2b81faab257832d3dbd42947a884f7ec99685d18" 2b81faa]:
{{{
#!CommitTicketReference repository=""
revision="2b81faab257832d3dbd42947a884f7ec99685d18"
Fixed #28906 -- Removed unnecessary bool() calls.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28906#comment:3>

Reply all
Reply to author
Forward
0 new messages