Skip to content

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

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

S4lt5
Copy link

@S4lt5 S4lt5 commented Oct 26, 2022

🗣 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

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

^ 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.

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Add a tag or create a release.

@S4lt5
Copy link
Author

S4lt5 commented Oct 26, 2022

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 :)

@jsf9k
Copy link
Member

jsf9k commented Oct 27, 2022

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?

@dav3r
Copy link
Member

dav3r commented Oct 27, 2022

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?

@S4lt5 S4lt5 changed the title Address alter Domain class initializer to provide per-instance copies of lists, add test for this bug, alter old tests to remove workaround sets of list() params. 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. Oct 27, 2022
Copy link
Collaborator

@Pascal-0x90 Pascal-0x90 left a comment

Choose a reason for hiding this comment

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

This is awesome! Yeah that funky work around was not the cleanest. This approach makes the code using the object cleaner.

@Pascal-0x90
Copy link
Collaborator

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?

@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.

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.

Creation of tests
4 participants