Modify Domain class initializer to provide fresh copies of default empty list, add test for this bug, alter old tests to remove workaround sets of list() params. #47
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🗣 Description
Per issue #1 and #3, the Domain class was sharing instances of the same list based on the 'typing' initializer definition. Specifically the '[]' is evaluated at the class level and thus shared amongst all instances, since the list was not actually being created within the init function.
I've fixed this by adding an 'or' statement that adds a default value when no value is passed (from my previous experience, this supersedes the typing definition)
💭 Motivation and context
When I first checked out this code and saw the set of
list(),list(),list()
I felt that this was something that could and should be fixed as often are things that require such workarounds. It turns out that it was a pretty simple change and cleans up the tests IMO.This avoids any issues where a user of the module is manipulating multiple Domains and accidentally pollutes eachother's lists.
This specifically closes #3 and #1
🧪 Testing
I added a new test_domain_init and fixed up previous tests that had the workaround list(),list(),list()... set of args.
✅ Pre-approval checklist
in code comments.
to reflect the changes in this PR.
^ Regarding above, the tests currently do not pass, that are not directly relevant to this issue, so unsure on how to proceed.
✅ Pre-merge checklist
No dependencies were modified from the default branches.
✅ Post-merge checklist