BDE-2483 - Move set_definition method to container#27
Conversation
| @@ -40,6 +40,14 @@ def setUp(self) -> None: | |||
| Tag(identifier="test_tag_B", value=sentinel.test_tag_B), | |||
There was a problem hiding this comment.
You copied the variable definition_e from test_builder to test_container, but it was already present in test_container under the name definition_d (now definition_d and definition_e have the same content). Could you modify definition_d so that it holds the value of the variable that is actually missing from test_builder (called definition_d in test_builder)?
That is, change the actual content of definition_d in test_container to:
self.definition_d = Definition(
"example.D",
"tests.resources.class_c.C",
[
Argument.no_kw_argument("@example.A"),
Argument("kw_arg", "test_param"),
Argument("b", "@example.B"),
],
{
Tag(identifier="test_tag_A", value=sentinel.test_tag_A_value),
},
)There was a problem hiding this comment.
I suggest changing the value of definition_d instead of definition_e to preserve the original order of attribute declarations in test_builder.
So, with this change, you should update the references in test_container from definition_d to definition_e (since their contents will have been swapped), and also update the references from value=sentinel.test_tag_A and value=sentinel.test_tag_B to value=sentinel.test_tag_A_value and value=sentinel.test_tag_B_value, which are more descriptive and accurate.
There was a problem hiding this comment.
I'd leave definition_d as it is. That way we don't have to edit more tests. And then change definition_e to be like the container_builder version of definition_d, so we kind of swap the contents of the definitions.
There was a problem hiding this comment.
I agree on the sentinel values, I'll change those too
BDE-2483
This PR is part of a series
💡 Context
While reviewing the library to do a talk about pypendency I have found some possible issues.
The
set_definitionfunction of ContainerBuilder makes it so that we are not following the single responsability principle. We could perfectly move this function to Container since it also handles definitions.This way the only purpose of ContainerBuilder is being a singleton of Container.
And Container has all the code related to managing services, definitions and tags.
📖 Summary
This PR moves the set_definition function from ContainerBuilder to Container.
Note: The tests still pass since ContainerBuilder inherits from Container but we should probably move the set_definition tests to the Container test suite
🧪 How should this be manually tested?
You can manually run the tests with: