Skip to content

Bug: Do not **unintentionally** modify function arguments #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
igalic opened this issue Aug 28, 2017 · 3 comments
Closed

Bug: Do not **unintentionally** modify function arguments #38

igalic opened this issue Aug 28, 2017 · 3 comments
Labels

Comments

@igalic
Copy link
Collaborator

igalic commented Aug 28, 2017

We have a handful of functions that take non-None arguments, and then modify them.
This leads to python directly modifying the passed argument, and is generally considered bad form.

One such example is JailConfigAddresses#add()

    def add(self, nic, addresses=[], notify=True):

        if isinstance(addresses, str):
            addresses = [addresses]

maybe there are linter params that will warn us of such code, until then we might need human eyes.

@gronke gronke added the bug label Aug 28, 2017
@gronke
Copy link
Member

gronke commented Aug 28, 2017

Agreed. Needs to be addressed! Should not be too complicated to fix, but a bit hard to spot. Is there a static code analyzer that does check for it?

@igalic
Copy link
Collaborator Author

igalic commented Aug 28, 2017

i don't know…
it doesn't seem to be in pylint, https://pylint.readthedocs.io/en/latest/technical_reference/extensions.html
and we already use flake8, which dosen't seem to catch those either: http://flake8.pycqa.org/en/latest/user/error-codes.html


we should definitely (also?) consider using MyPy.
This one will also be able to check our type hints (#39).

igalic added a commit to igalic/libioc that referenced this issue Aug 28, 2017
a non-None default causes the parameters to be pass-by-reference, and
hence mutable! We don't generally want that.

This fixes bsdci#38
igalic added a commit to igalic/libioc that referenced this issue Aug 28, 2017
a non-None default causes the parameters to be pass-by-reference, and
hence mutable! We don't generally want that.

This partially(??) addresses bsdci#38
gronke pushed a commit that referenced this issue Aug 28, 2017
a non-None default causes the parameters to be pass-by-reference, and
hence mutable! We don't generally want that.

This partially(??) addresses #38
@gronke gronke added chore and removed bug labels Sep 4, 2017
@igalic
Copy link
Collaborator Author

igalic commented Sep 17, 2017

after installing flake8-mutable, i get the following:

./iocage/lib/Network.py:36:33: M511 - mutable default arg of type List
./iocage/lib/DevfsRules.py:33:47: M511 - mutable default arg of type List
./iocage/lib/Jail.py:186:63: M511 - mutable default arg of type Dict
./iocage/lib/Filter.py:46:36: M511 - mutable default arg of type Call
./iocage/lib/NetworkInterface.py:34:33: M511 - mutable default arg of type List
./iocage/lib/NetworkInterface.py:35:33: M511 - mutable default arg of type List
./iocage/lib/NetworkInterface.py:43:33: M511 - mutable default arg of type List

igalic added a commit that referenced this issue Sep 17, 2017
install flake8-mutable (and, just to be sure, flake8-builtins;) to check
for these things, as reported in #38.
igalic added a commit that referenced this issue Sep 17, 2017
install flake8-mutable (and, just to be sure, flake8-builtins;) to check
for these things, as reported in #38.
gronke pushed a commit that referenced this issue Sep 19, 2017
install flake8-mutable (and, just to be sure, flake8-builtins;) to check
for these things, as reported in #38.
gronke pushed a commit that referenced this issue Sep 19, 2017
install flake8-mutable (and, just to be sure, flake8-builtins;) to check
for these things, as reported in #38.
@gronke gronke closed this as completed Sep 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants