{{{
def __init__(self, *, fields=[], name=None, db_tablespace=None):
}}}
The {{{fields}}} argument is set to a mutable object, which is usually
considered bad practice as it can lead to unexpected results:
* http://effbot.org/zone/default-values.htm
* http://docs.python-guide.org/en/latest/writing/gotchas/
There are some valid uses of mutable defaults, but I can't see any code
taking advantage of the mutability by looking at the code of the
{{{Index}}} class.
If there's no good reason for the default to be mutable, it should be
changed to:
{{{
def __init__(self, *, fields=None, name=None, db_tablespace=None):
if fields is None:
fields = []
# ...rest of the code
}}}
If there's a good reason, I think we should document it in a comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/29178>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:1>
Comment (by Tim Graham):
An older [https://github.com/django/django/pull/9368 PR] suggests
accepting a list or tuple and defaulting to `fields=()`. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:2>
* owner: nobody => Marc Kirkwood
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:3>
Comment (by Flavio Curella):
I think it should It should accept a list or a tuple as in
https://github.com/django/django/pull/9368
Re: defaulting to {{{()}}} vs defaulting to {{{None}}}, I don't really
have a strong opinion. I'm used to see the {{{arg=None}}} pattern more
often, but that could just be cargo-culting.
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:4>
Comment (by Marc Kirkwood):
Replying to [comment:4 Flavio Curella]: I agree with that; looks like the
PR you linked to solves the problem better.
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:5>
* owner: Marc Kirkwood => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:6>
Comment (by Flavio Curella):
@Tim Graham: I think we can just link
https://github.com/django/django/pull/9368 to this ticket and use it as it
is
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:7>
Comment (by Flavio Curella):
To elaborate: the only reason I think it should support both list and
tuples is because we have other places where we do so: for example,
{{{Meta.ordering}}} comes to mind.
But I'm with you if you'd rather not persevere with that pattern, and push
the user to use {{{tuples}}} whenever possible.
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:8>
* status: new => assigned
* owner: (none) => atmishra
Comment:
I would like to work on this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:9>
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
There's already a [https://code.djangoproject.com/ticket/29178 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:10>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"10c0fe528a2089f4ba206caa50f9a98f8d9c8a15" 10c0fe5]:
{{{
#!CommitTicketReference repository=""
revision="10c0fe528a2089f4ba206caa50f9a98f8d9c8a15"
Fixed #29178 -- Allowed Index.fields to accept a tuple.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:11>