Hello!
I noticed that the Configuration class has several default field values which are mutable (empty sets, and the ALL_RELEASE_TYPES).
In the (admittedly probably uncommon for Github Actions usage) case where multiple Configuration are instantiated, and they didn't have these fields overwritten by user input, they would all share mutations, similar to the "function parameter default value is mutable" issue.
Here's an example:
>>> import src.config as c
>>>
>>> j = c.Configuration()
>>> j.ignore_actions
set()
>>>
>>> k = c.Configuration.create({'INPUT_PULL_REQUEST_LABELS': 'label1,label2'})
>>> k.ignore_actions
set()
>>> k.ignore_actions.add('ignore1')
>>> j.ignore_actions # `j`'s copy should still be empty
{'ignore1'}
>>>
>>> k.pull_request_labels
{'label1', 'label2'}
>>> j.pull_request_labels
set()
>>> j.pull_request_labels.add('another_label')
>>> k.pull_request_labels
{'label1', 'label2'} # This is fine because `k` got a new set when parsing user input
>>>
>>>
>>> # The shared reference to `ALL_RELEASE_TYPES` has the same issue:
>>> j.release_types
['major', 'minor', 'patch']
>>> k.release_types.remove('minor')
>>> j.release_types
['major', 'patch']
pylint has an open issue for adding a warning about this: pylint-dev/pylint#3716
Resolution
I think two approaches to resolving this could be to either
-
use empty frozenset() as defaults, instead of empty set(). This would mean that users of Configuration would no longer be able to call, for example, config.ignore_actions.add(...), if they were doing that.
-
or convert Configuration from a NamedTuple into a @dataclass(frozen=True). Dataclass fields can have a factory function to return a new empty set for each instance, which would solve the reference sharing while maintaining internal mutability, if that's what you want.
Hello!
I noticed that the
Configurationclass has several default field values which are mutable (empty sets, and theALL_RELEASE_TYPES).In the (admittedly probably uncommon for Github Actions usage) case where multiple
Configurationare instantiated, and they didn't have these fields overwritten by user input, they would all share mutations, similar to the "function parameter default value is mutable" issue.Here's an example:
pylinthas an open issue for adding a warning about this: pylint-dev/pylint#3716Resolution
I think two approaches to resolving this could be to either
use empty
frozenset()as defaults, instead of emptyset(). This would mean that users ofConfigurationwould no longer be able to call, for example,config.ignore_actions.add(...), if they were doing that.or convert
Configurationfrom a NamedTuple into a@dataclass(frozen=True). Dataclass fields can have a factory function to return a new empty set for each instance, which would solve the reference sharing while maintaining internal mutability, if that's what you want.