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

Change mutable default arguments to None #6376

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

bwroblew
Copy link
Contributor

@bwroblew bwroblew commented Jan 9, 2023

Reading PyG code I accidently saw that class and noticed that it uses mutable default arguments what is generally considered a bad practice and can lead to a lot of problems. I believe it's not a desired behaviour (am I wrong?).

Problems can occur when the class is extended in the future such that it changes the value of any of those lists, the default value for new instances of this class would also be affected. The other possibility is that user changes any of the follow_batch, exclude_keys fields (those doesn't start with an underscore, so it's kinda possible) after the object is initialized, what's also going to change the default value for new instances.

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #6376 (279e948) into master (8e4f967) will not change coverage.
The diff coverage is n/a.

❗ Current head 279e948 differs from pull request most recent head 9de1c8b. Consider uploading reports for the commit 9de1c8b to get more accurate results

@@           Coverage Diff           @@
##           master    #6376   +/-   ##
=======================================
  Coverage   84.52%   84.52%           
=======================================
  Files         393      393           
  Lines       21563    21563           
=======================================
  Hits        18227    18227           
  Misses       3336     3336           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@rusty1s rusty1s enabled auto-merge (squash) January 9, 2023 16:17
@rusty1s rusty1s changed the title Change mutable default arguments to None Change mutable default arguments to None Jan 9, 2023
@rusty1s rusty1s disabled auto-merge January 9, 2023 16:17
@rusty1s rusty1s enabled auto-merge (squash) January 9, 2023 16:17
@rusty1s rusty1s merged commit 19d5cbc into pyg-team:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants