Skip to content

BDE-2483 - Move set_definition method to container#27

Merged
LynxEko merged 4 commits intomasterfrom
feature/BDE-2483_move_set_definition_to_container
Jun 10, 2025
Merged

BDE-2483 - Move set_definition method to container#27
LynxEko merged 4 commits intomasterfrom
feature/BDE-2483_move_set_definition_to_container

Conversation

@LynxEko
Copy link
Copy Markdown
Contributor

@LynxEko LynxEko commented Jun 2, 2025

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_definition function 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:

docker build . -t pypendency-dev
docker run -v $(pwd)/.:/usr/src/app pypendency-dev bash -c "pipenv run make run-tests"

@LynxEko LynxEko requested review from andrsdt, eduvives and marcoshj June 2, 2025 08:58
@LynxEko LynxEko self-assigned this Jun 2, 2025
@LynxEko LynxEko requested a review from a team as a code owner June 2, 2025 08:58
Comment thread tests/test_container.py Outdated
@@ -40,6 +40,14 @@ def setUp(self) -> None:
Tag(identifier="test_tag_B", value=sentinel.test_tag_B),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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),
            },
        )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree on the sentinel values, I'll change those too

@LynxEko LynxEko requested a review from eduvives June 4, 2025 09:53
Copy link
Copy Markdown
Contributor

@andrsdt andrsdt left a comment

Choose a reason for hiding this comment

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

Great refactor! 🥇🔝

@LynxEko LynxEko merged commit 54abfe2 into master Jun 10, 2025
3 checks passed
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.

3 participants