0

My code triggers a warning in pylint:

def getInsertDefault(collection=['key', 'value'], usefile='defaultMode.xml'):
    return doInsert(collection,usefile,True)

The warning is pretty clear, it's Mutable Default Arguments, I'm getting the point in several instances it can give a wrong impression of what's happening. There are several posts on SA already, but it doesn't feel this one here is covered. Most questions and examples deal with empty lists which are weak-referenced and can cause an error.

I'm also aware it's better practice to change the code to getInsertDefault(collection=None ...) but in this method for default-initialization, I don't intend to do anything with the list except reading, (why) is my code dangerous or could result in a pitfall?

--EDIT--

To the point: Why is the empty dictionary a dangerous default value in Python? would be answering the question. Kind of: I am aware my code is against the convention and could result in a pitfall - but in this very specific case: Am I safe?

I found the suggestion in the comments useful to use collection=('key', 'value') instead as it's conventional and safe. Still, out of pure interest: Is my previous attempt able to create some kind of major problem?

Qohelet
  • 1,459
  • 4
  • 24
  • 41
  • 1
    Does this answer your question? [Why is the empty dictionary a dangerous default value in Python?](https://stackoverflow.com/questions/26320899/why-is-the-empty-dictionary-a-dangerous-default-value-in-python) – metatoaster Apr 20 '20 at 10:36
  • 1
    Since you appear to only want some sequence of elements, consider using a tuple instead (i.e. `getInsertDefault(collection=('key', 'value'), ...)`) as tuples are immutable. – metatoaster Apr 20 '20 at 10:38
  • @metatoaster - your first answer is "kind of" an answer. I interpret it as: Nothing can happen in my case, but it's bad practice because it can lead to errors in some cases. Is that correct? I want to make sure I'm not overseeing anything fundamental. – Qohelet Apr 20 '20 at 11:02
  • @metatoaster - your 2nd answer is actually helpful. It's a decent alternative and I implemented it already – Qohelet Apr 20 '20 at 11:03
  • 1
    @Qohelet you _are_ overseeing something fundamental : code tends to evolve with time. See my answer for more. – bruno desthuilliers Apr 20 '20 at 11:55
  • @brunodesthuilliers - this answer I can gladly accept – Qohelet Apr 20 '20 at 12:00

1 Answers1

1

Assuming that doInsert() (and whatever code doInsert is calling) is only ever reading collection, there is indeed no immediate issue - just a time bomb.

As soon as any part of the code seeing this list starts mutating it, your code will break in the most unexpected way, and you may have a hard time debugging the issue (imagine if what changes is a 3rd part library function tens of stack frames away... and that's in the best case, where the issue and it's root cause are still in the same direct branch of the call stack - it might stored as an instance attribute somewhere and mutated by some unrelated call, and then you're in for some fun).

Now the odds that this ever happens are rather low, but given the potential for subtles (it might just result in incorrect results once in a while, not necessarily crash the program) and hard to track bugs this introduces, you should think twice before assuming that this is really "safe".

bruno desthuilliers
  • 75,974
  • 6
  • 88
  • 118