Fix/circular dependencies#620
Conversation
Wuestengecko
left a comment
Wuestengecko
left a comment
There was a problem hiding this comment.
Hi, and thanks for the PR!
Circular dependencies in present code prevent effective codebase analysis
The tools we normally use in this repo (most notably mypy and ruff) seem to be handling these cycles just fine. Which other tools are you using where you're seeing issues? Considering the implications about API surface (which I mentioned in a comment below), would it be an option to fix those tools and make them properly handle cycles instead?
Additionally, this PR doesn't fix all cycles - we're still importing capellambse.metamodel into capellambse.model._obj (though guarded behind if TYPE_CHECKING). I didn't fully check the codebase, so there may or may not be other cycles left.
| import typing as t | ||
|
|
||
| import capellambse.model as m | ||
| from capellambse.model import _descriptors, _obj, _pods |
There was a problem hiding this comment.
From the perspective of capellambse.metamodel, the capellambse.model._descriptors and other "underscored" modules are private to model and should not be used directly. This is why we currently use them via the public exports at capellambse.model.
If we want to directly import _descriptors etc. we need to make them public by removing the leading underscore. However this would also increase the public API surface, which is something I'd want to avoid, unless there's a very good reason.
| The output format to use. | ||
|
|
||
| If ``None``, the :class:`~capellambse.diagram.Diagram` is returned | ||
| If ``None``, the :class:`~capellambse._diagram.Diagram` is returned |
| sourcefmt | ||
| Name of the current format. If None, the current format is a | ||
| :class:`capellambse.diagram.Diagram` object. | ||
| :class:`capellambse._diagram.Diagram` object. |
| from capellambse import aird, helpers, svg | ||
| from capellambse import diagram as _diagram |
There was a problem hiding this comment.
Why this import rename? There doesn't seem to be any naming conflict.
| origin = m.Association["CatalogElement"]((NS, "CatalogElement"), "origin") | ||
| current_compliancy = m.Association["CompliancyDefinition"]( | ||
|
|
||
| kind = _pods.EnumPOD("kind", CatalogElementKind) | ||
| author = _pods.StringPOD("author") | ||
| environment = _pods.StringPOD("environment") | ||
| suffix = _pods.StringPOD("suffix") | ||
| purpose = _pods.StringPOD("purpose") | ||
| is_read_only = _pods.BoolPOD("readOnly") | ||
| version = _pods.StringPOD("version") | ||
| tags = _pods.StringPOD("tags") | ||
| origin = _descriptors.Single( | ||
| _descriptors.Association["CatalogElement"]( | ||
| (NS, "CatalogElement"), "origin" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This change from Association to Single[Association] might be interesting (and small) enough to put on a faster track.
|
|
||
| LOGGER = logging.getLogger(__name__) | ||
|
|
||
| import capellambse.metamodel as mm |
There was a problem hiding this comment.
- This import should be moved to the other imports a few lines above (before the
isort:skip-ed pathlib import) - We're also importing several metamodel submodules directly (only to use them exactly once each), we can easily move these over to
mm.*as well
| import enum | ||
|
|
||
| import capellambse.model as m | ||
| from capellambse.metamodel.behavior import AbstractBehavior, AbstractSignal |
There was a problem hiding this comment.
| self, | ||
| model: m.MelodyModel, | ||
| parent: etree._Element, | ||
| xmltag: str | None, |
There was a problem hiding this comment.
Another bug fix, might also be worth fast-tracking (note we also need to actually pass it on to super().__init__(), which we currently don't).
| "ownedConfigurationItems", (NS, "ConfigurationItem") | ||
| ) | ||
| configuration_item_pkgs = m.Containment["ConfigurationItemPkg"]( | ||
| configuration_item_pkgs: _descriptors.Containment[ConfigurationItemPkg] = _descriptors.Containment["ConfigurationItemPkg"]( |
There was a problem hiding this comment.
Explicit annotations should not be necessary as long as the Generic does get passed somewhere.
| configuration_item_pkgs: _descriptors.Containment[ConfigurationItemPkg] = _descriptors.Containment["ConfigurationItemPkg"]( | |
| configuration_item_pkgs = _descriptors.Containment["ConfigurationItemPkg"]( |
| import typing as t | ||
|
|
||
| import capellambse.model as m | ||
| from capellambse.model import _descriptors, _obj, _pods |
There was a problem hiding this comment.
These files are in large parts generated automatically based on Capella's upstream metamodel using scripts/ecore2py.py. That script currently still uses classes from capellambse.model directly, and would have to be adjusted to use these new imports as well.
2 participants
Circular dependencies in present code prevent effective codebase analysis, this change switches import from
capellambse.modelto more specific dependencies to address this issue.