Fix "dangerous default" mutable paramters and enable pylint warning for future.

[myxie]

Issue

Our code has a few instances of initialising empty list/dicts in the method parameters:

def myfunc(arg=1, param=[]):
    param.append(arg)
    print(param)

The problem here is that in Python, the default value for param is initialised when the function is first interpreted, and it is mutable. This means that if we do not pass in values for param as an argument, the following happens:

myfunc(5) # > [5]
myfunc(6) # > [5, 6]

We are adding to the same default-initalised param object, rather than creating a new default each time the function is called.

This has been a common 'gotcha' in Python for many years: https://web.archive.org/web/20200221224620id_/http://effbot.org/zone/default-values.htm

Solution

The Pythonic solution to this is to make sure that the default for a parameterised mutable is None, and then initialise an empty
data structure in the function:

def myfunc(arg=1, param=None):
    if not param:
        param = [] 
    param.append(arg)
    print(param)

Summary by Sourcery

This pull request addresses the issue of mutable default parameters in function definitions by initializing them to None and setting them within the function. Additionally, it enables a pylint warning to prevent future occurrences of this issue.

• Bug Fixes:
  • Fixed instances of mutable default parameters in function definitions to prevent unintended side effects by initializing them to None and setting them within the function.
• Enhancements:
  • Enabled pylint warning for mutable default parameters to prevent future occurrences of this issue.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request addresses the issue of 'dangerous default' mutable parameters in the codebase. The changes involve replacing default mutable parameters (like lists and dictionaries) with None and initializing them within the method if they are None. This ensures that a new object is created each time the function is called, preventing unintended side effects. Additionally, the pull request enables pylint warnings to catch such issues in the future.

File-Level Changes

Files	Changes
daliuge-engine/dlg/testutils.py daliuge-engine/test/manager/test_dm.py daliuge-engine/dlg/manager/session.py daliuge-engine/dlg/manager/composite_manager.py daliuge-engine/dlg/droputils.py daliuge-common/dlg/clients.py daliuge-engine/dlg/manager/node_manager.py daliuge-common/dlg/restutils.py daliuge-engine/dlg/manager/drop_manager.py daliuge-engine/dlg/manager/replay.py	Changed default mutable parameters to None and added initialization within the method. Added checks for None before initializing empty lists or dictionaries.

---

Tips

• Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
• Continue your discussion with Sourcery by replying directly to review comments.
• You can change your review settings at any time by accessing your dashboard:
  • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
  • Change the review language;
• You can always contact us if you have any questions or feedback.

[sourcery-ai]

Hey @myxie - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟡 General issues: 3 issues found
• 🟢 Security: all looks good
• 🟡 Testing: 1 issue found
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

[sourcery-ai]

suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)

Suggested change

	visited = visited if visited else []
	visited = visited or []

Explanation

Here we find ourselves setting a value if it evaluates to True, and otherwise
using a default.

The 'After' case is a bit easier to read and avoids the duplication of
input_currency.

It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.

[coveralls]

coverage: 79.589%. remained the same
when pulling 6a0d211 on LIU-383_DangerousDefaults
into f2a1e7f on master.

[myxie]

Hi @awicenec, another small PR for you when you have the time.

[myxie]

@awicenec if you're happy with these changes I will go ahead and merge :)

[awicenec]

If tests are passing good to merge.