Opened 10 years ago
Closed 10 years ago
#23626 closed Cleanup/optimization (wontfix)
Change coding style for sql, params return lines
Reported by: | Marc Tamlyn | Owned by: | Anna Warzecha |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | afraid-to-commit |
Cc: | amizya@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
The current style can be quite confusing: return '%s @> %s' % (lhs, rhs), params
This is much clearer:
sql = '{} @> {}'.format(lhs, rhs) params = lhs_params + rhs_params return sql, params
We should do this everywhere in the ORM or not at all.
(Originally brought up by Aymeric on https://github.com/django/django/pull/3219)
Change History (10)
comment:1 by , 10 years ago
Easy pickings: | set |
---|
comment:2 by , 10 years ago
comment:3 by , 10 years ago
This ticket was opened two days ago and there are no commits along those lines I can see. For example I mean places like:
https://github.com/django/django/blob/bc46e4d4fa61eead13fe58048ae646f07d632e4f/django/db/models/lookups.py#L149-L154
https://github.com/django/django/blob/92a17eaae081a213171b044858d6fc29df2df733/django/contrib/postgres/fields/array.py#L167-L172
There aren't that many, and several are clearer to read than the examples in that PR. The docs however suggest the current style used. It may be worth looking more generally at string manipulation in the ORM and using .format()
instead of %s
notation.
comment:4 by , 10 years ago
@mjtamlyn should I create a new ticket for the formatting part in the ORM ?
comment:5 by , 10 years ago
Cc: | added |
---|
comment:6 by , 10 years ago
Keywords: | afraid-to-commit added |
---|
I've marked this ticket as especially suitable for people following the Don't be afraid to commit tutorial at the PyCon Ireland 2014 sprints. If you're tackling this ticket, please don't hesitate to ask me for guidance if you'd like any, either here or on the Django IRC channels, where I can be found as EvilDMP.
comment:7 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 10 years ago
Owner: | changed from | to
---|
comment:9 by , 10 years ago
There are quite a lot places where %s
is used in ORM string manipulation and I am not quite sure that changing to format is a good idea. It's actually not helping much with readability especially in base SQL strings defined in db/backend/schema.py
. Curly braces are also used in T-SQL for escaping Date, Time and Timestamp, and in PostgreSQL for supplying Array values - this wouldn't make the issue an easy picking one IMO.
I am not really sure if changing that would have any positive outcome. I would just abandon the idea.
comment:10 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
I am inclined to agree. Thanks for doing the detailed research.
This seems to be already taken care of, pretty sure it can be closed.