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
Conversation
|
I'd like to make sure that this is welcome before I go any further. I saw there was a bit in there on "all tests pass" when out of the box more than a few fail, so before I dive significantly into this I'd like to make sure someone has the lights on and is monitoring this project :) |
|
Thanks for the contribution @Yablargo! I'm not sure who is in charge of this particular project. @cisagov/team-ois or @KyleEvers, do you know? |
I'm not sure if @Pascal-0x90 is still involved in this project, but I know they were involved at the start. @Pascal-0x90, are you still involved here or can you point us to someone who is/can be? |
Pascal-0x90
left a comment
There was a problem hiding this comment.
This is awesome! Yeah that funky work around was not the cleanest. This approach makes the code using the object cleaner.
@dav3r I am technically not from a CISA perspective but I have been neglecting the repo unfortunately. I am going to make a more conscious effort to help with this repo. I don't have merging ability but I will go through and review PRs and issues. |
🗣 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