Remove dangerous default argument

[pnijhara]

Hi,
It is good not to use the default arguments while calling a function.

MCVE Code Sample

In file xarray/tests/test_conventions.py

# Your code here
329    @contextlib.contextmanager
330    def roundtrip(
331        self, data, save_kwargs={}, open_kwargs={}, allow_cleanup_failure=False332    ):
333        store = CFEncodedInMemoryStore()
334        data.dump_to_store(store, **save_kwargs)

Problem Description

Default arguments in Python are evaluated once when the function definition is executed. This means that the expression is evaluated only once when the function is defined and that the same value is used for each subsequent call. So if you are modifying the mutable default argument — a list, dict, etc., it will be modified for all future calls.

[pnijhara]

Same can also be found in xarray/tests/test_backends.py

3621    ny=3,
3622    nz=3,
3623    transform=None,
3624    transform_args=[5000, 80000, 1000, 2000.0],3625    crs={"units": "m", "no_defs": True, "ellps": "WGS84", "proj": "utm", "zone": 18},
3626    open_kwargs=None,
3627    additional_attrs=None,

[shoyer]

Yes, this would be nice to clean up, but mostly just for the sake of writing idiomatic Python. We don't ever modify any of these arguments.

[max-sixty]

We would take a PR to change these to default None and then x=x or {}

[shoyer]

We would take a PR to change these to default None and then x=x or {}

I like this pattern a little better, because it's clear it's comparing to a default value:

if x is None:
    x = {}

[DocOtak]

Slightly related, I've noticed a bunch of assert statements outside the testing paths e.g. the __init__ for xr.DataArray has 3 of them. Would that be something to fix up as well?

[max-sixty]

I might be behind the curve on the pythonic approach there: what's wrong with asserts there?

[shoyer]

These are appropriate uses for assert (verifying internal invariants). Please keep them.

[pnijhara]

We would take a PR to change these to default None and then x=x or {}

I like this pattern a little better, because it's clear it's comparing to a default value:

if x is None:
    x = {}

@shoyer I was thinking of the same. Will send a PR with this change.

[DocOtak]

My concern was due to python not evaluating asserts if "optimization" is requested.

[pnijhara]

I might be behind the curve on the pythonic approach there: what's wrong with asserts there?

As far as I know, assert should not be used outside of tests because while running python with -O (optimize) flag, it will remove the assert statements.

Will open a separate issue for this. We should have a discussion over asserts over there.

[max-sixty]

OK, I think that's fairly rare, but depending on @shoyer 's preferences, changing those to a check and raise seems reasonable to me

[shoyer]

assert is the appropriate way to verify internal invariants. We use it routinely at Google and you can also find many examples in Python’s standard library:
https://github.com/python/cpython/search?l=Python&q=assert

[pnijhara]

@shoyer, I partially agree with your statement that " assert is the appropriate way to verify internal invariants". This is correct but not every time. When you see the link that you shared it also shows that most of the assert statements are used in tests and not in simple files.

Let me quote again
"Since assert provides an easy way to check some condition and fail execution, it’s very common for developers to use it to check validity. But when the Python interpreter is invoked with the -O (optimize) flag, the assert statements are removed from the bytecode. So, if assert statements are used for user-facing validation in production code, the block won’t be executed at all — potentially opening up a security vulnerability. It is recommended to use assert statements only in tests."

Again it's just a suggestion IMHO

[shoyer]

We do not use assert for user facing validation

…

On Sat, Apr 25, 2020 at 3:37 AM Prajjwal Nijhara ***@***.***> wrote: @shoyer <https://github.com/shoyer>, I partially agree with your statement that " assert is the appropriate way to verify internal invariants". This is correct but not every time. When you see the link that you shared it also shows that most of the assert statements are used in tests and not in simple files. Let me quote again "Since assert provides an easy way to check some condition and fail execution, it’s very common for developers to use it to check validity. But when the Python interpreter is invoked with the -O (optimize) flag, the assert statements are removed from the bytecode. So, if assert statements are used for user-facing validation in production code, the block won’t be executed at all — potentially opening up a security vulnerability. It is recommended to use assert statements only in tests." Again it's just a suggestion IMHO — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4002 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJFVWLOJULCHNMC5F4KZTROK4NZANCNFSM4MQL3K7A> .

[pnijhara]

Agreed with that. So, should I open another issue for asserts or not?

[shoyer]

Don't waste your time on that one

…

On Sat, Apr 25, 2020 at 9:19 AM Prajjwal Nijhara ***@***.***> wrote: Agreed with that. So, should I open another issue for asserts or not? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4002 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJFVVVF7RW5HZFKAF62X3ROMERVANCNFSM4MQL3K7A> .