Skip to content
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

NFC: Extract default key variable #58

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Conversation

blarz
Copy link
Contributor

@blarz blarz commented Oct 21, 2019

No description provided.

@blarz blarz requested a review from chca42 October 21, 2019 19:20
@chca42
Copy link
Contributor

chca42 commented Oct 24, 2019

There is a reason why I use key=None in this case (you can still extract the key as global variable, though): If a default argument in python is a mutable sequence, the instance passed in the function is only created once at startup and every function call receives exactly the same instance (default arguments are evaluated upon processing the function definition). Thus you must not modify this variable in this case. Since python does not allow defining this as constant, it is dangerous to assume this implicitly to prevent side effects if any code part passes this by reference somewhere and the key array is modified without being copied before. Since this behavior of python (which is exactly the way it is defined in the language specification, see https://docs.python.org/3/reference/compound_stmts.html#function-definitions at the paragraph "Default parameter values ...") is somewhat surprising, my personal feeling is to prevent anyone from running into this issue.

If you want to extract the key, my recommendation would be to copy the global variable into the argument with "xyz.copy()" if detected as None.

@blarz
Copy link
Contributor Author

blarz commented Oct 28, 2019

That makes sense, I did not think about that!
I updated the PR accordingly.

@chca42
Copy link
Contributor

chca42 commented Oct 28, 2019

thx

@chca42 chca42 merged commit 81f27ff into k4cg:master Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants