#12308: Adding tablespace support to postgres backends
-------------------------------------+-------------------------------------
Reporter: tclineks | Owner: aaugustin
Type: New | Status: new
feature | Component: Database layer
Milestone: | (models, ORM)
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Thanks for your review!
Regarding the confusing code pattern, in fact, the two blocks aren't
equivalent. The goal of this construct is to add a space at the beginning
of `tablespace_sql` only when it isn't the empty string. With your
version, if `self.connection.ops.tablespace_sql(tablespace)` returns `''`,
then `tablespace_sql` is set to `' '`, and this results in an extra space
in the final SQL.
Currently, Django contains this pattern:
{{{
if tablespace:
sql = self.connection.ops.tablespace_sql(tablespace)
if sql:
tablespace_sql = ' ' + sql
else:
tablespace_sql = ''
else:
tablespace_sql = ''
}}}
Which my patch condenses to:
{{{
if tablespace:
tablespace_sql = self.connection.ops.tablespace_sql(tablespace)
if tablespace_sql:
tablespace_sql = ' ' + tablespace_sql
else:
tablespace_sql = ''
}}}
Would it be more explicit like this?
{{{
if tablespace:
tablespace_sql = self.connection.ops.tablespace_sql(tablespace)
else:
tablespace_sql = ''
if tablespace_sql:
tablespace_sql = ' ' + tablespace_sql
}}}
(It's slightly less efficient because the second `if` isn't necessary when
`tablespace == ''`.)
Should I just keep the original pattern?
No strong feelings here, I settled for the shorter version because they
all looked equivalent to me...
----
Regarding the "features" flags, some are computed by a the `confirm()`
method, which is only called from `create_test_db()`. Therefore, I think
that `connection.features` should only be used for the tests
(`@skipIfDBFeature` / `@skipUnlessDBFeature`). This rule isn't written
anywhere, but it's comforted by the fact that `connection.features` isn't
used anywhere within `django.db.backends`.
Full story: in my initial implementation (with MySQL), support for
transactions was computed by `confirm()` because it depended on the
version of MySQL, and I then used `features.supports_transactions` to
generate the appropriate SQL. It worked perfectly in the tests, and failed
everywhere else because `confirm()` wasn't called, so
`features.supports_transaction` was always `None`.
----
I leave "Patch needs improvement" set until we reach an agreement on the
first point :)
--
Ticket URL: <https://code.djangoproject.com/ticket/12308#comment:18>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
