Skip to content
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

Removed the __all__ declaration from the griptape.mixins module. #1164

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

collindutter
Copy link
Member

Describe your changes

Griptape often uses __all__ in its packages to simplify module imports for users. However, this convenience comes with a tradeoff: all modules within the package must be imported to be included in __all__. While this is usually not an issue, it can lead to circular dependency problems in certain cases.

The griptape.mixins package is particularly prone to these issues because its modules aren't restricted to specific parts of the framework. As a result, mixin modules can easily introduce circular dependencies. For instance, simply adding a J2 import to Rule can trigger the following error:

ImportError while loading conftest '/Users/collindutter/Documents/griptape/griptape/tests/unit/conftest.py'.
tests/unit/conftest.py:3: in <module>
    from tests.mocks.mock_drivers_config import MockDriversConfig
tests/mocks/mock_drivers_config.py:3: in <module>
    from griptape.configs.drivers import DriversConfig
griptape/configs/__init__.py:1: in <module>
    from .base_config import BaseConfig
griptape/configs/base_config.py:7: in <module>
    from griptape.mixins.serializable_mixin import SerializableMixin
griptape/mixins/__init__.py:4: in <module>
    from .rule_mixin import RuleMixin
griptape/mixins/rule_mixin.py:7: in <module>
    from griptape.rules import Rule, Ruleset
griptape/rules/__init__.py:1: in <module>
    from griptape.rules.rule import Rule
griptape/rules/rule.py:5: in <module>
    from griptape.utils.j2 import J2
griptape/utils/__init__.py:6: in <module>
    from .command_runner import CommandRunner
griptape/utils/command_runner.py:5: in <module>
    from griptape.artifacts import BaseArtifact, ErrorArtifact, TextArtifact
griptape/artifacts/__init__.py:1: in <module>
    from .base_artifact import BaseArtifact
griptape/artifacts/base_artifact.py:10: in <module>
    from griptape.mixins import SerializableMixin
E   ImportError: cannot import name 'SerializableMixin' from partially initialized module 'griptape.mixins' (most likely due to a circular import) (/Users/collindutter/Documents/griptape/griptape/griptape/mixins/__init__.py)

This PR addresses the issue by removing __all__ from griptape.mixins and updating all imports to reference the specific module directly. While this change is technically breaking, I haven't included a migration guide since most users are unlikely to use mixins directly.

Issue ticket number and link

NA

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@collindutter collindutter merged commit 9d9b643 into dev Sep 10, 2024
15 checks passed
@collindutter collindutter deleted the refactor/mixins branch September 10, 2024 23:09
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.

2 participants