New datamodels#50
Conversation
0cc33eb to
aec1ae5
Compare
|
@josemmo maybe you can set the |
|
@gonzalocasas Neat! I was waiting to merge that first one, but this is better. |
There was a problem hiding this comment.
Pull request overview
Introduces new datamodels (Project, elements.Element) along with a generic KeyValueStorage abstraction, an IfcUUID helper, Cadwork↔COMPAS converter helpers, and a comprehensive pytest suite that mocks the Cadwork API. Existing datamodel/scene/storage/conversion APIs are marked with @deprecated to steer consumers to the new modules.
Changes:
- New
compas_cadwork.Projectandcompas_cadwork.elements.Elementclasses built on a genericKeyValueStorage/IterableKeyValueStoragefor attributes/data, plus a newIfcUUIDhelper for Base64-compressed IFC GUIDs. - New unit tests with a
CadworkMocksfixture intests/conftest.pythat injects mocked Cadwork modules intosys.modules. - Legacy APIs (datamodel, scene, storage, conversions) annotated with
@deprecatedfromtyping_extensions.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/compas_cadwork/init.py | Re-exports Project. |
| src/compas_cadwork/project.py | New Project datamodel with attribute/data accessors. |
| src/compas_cadwork/elements/init.py | Re-exports Element. |
| src/compas_cadwork/elements/element.py | New Element class wrapping Cadwork element APIs. |
| src/compas_cadwork/utils/init.py | Re-exports IfcUUID. |
| src/compas_cadwork/utils/ifc_uuid.py | IfcUUID with Base64-compressed encoding. |
| src/compas_cadwork/utils/storage.py | Generic KeyValueStorage / IterableKeyValueStorage mixins. |
| src/compas_cadwork/utils/converters.py | New point/vector ↔ Cadwork converters. |
| src/compas_cadwork/conversions/primitives.py | Existing converters marked @deprecated. |
| src/compas_cadwork/datamodel/element.py | Element, ElementGroup, ElementGroupingType marked @deprecated. |
| src/compas_cadwork/datamodel/dimension.py | AnchorPoint, Dimension marked @deprecated. |
| src/compas_cadwork/scene/{scene,beamobject,camera,instructionobject}.py | Scene objects/types marked @deprecated. |
| src/compas_cadwork/storage.py | Storage classes marked @deprecated. |
| src/compas_cadwork/utilities/{events,ifc_export}.py | Helpers marked @deprecated. |
| tests/conftest.py | Adds CadworkMocks fixture stubbing Cadwork modules. |
| tests/compas_cadwork/test_project.py | Unit tests for Project. |
| tests/compas_cadwork/elements/test_element.py | Unit tests for Element. |
| tests/compas_cadwork/utils/test_ifc_uuid.py | Tests for IFC Base64 encoding. |
| tests/compas_cadwork/utils/test_storage.py | Tests for storage abstractions. |
| tests/test_placeholder.py | Removed placeholder test. |
| CHANGELOG.md | Records the new classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gonzalocasas
left a comment
There was a problem hiding this comment.
I can't comment much on the cadwork data model change itself, but I think we should reconsider the drift from conventions like the conversions stuff and maybe also scene
| from typing_extensions import deprecated | ||
|
|
||
|
|
||
| @deprecated("Use compas_cadwork.utils.converters.compas_to_cwpoint instead") |
There was a problem hiding this comment.
Wait... this goes against the convention in compas that all conversions are under conversions and are in the form of [element_or_class_name]_to_[target_environment_or_compas] so point_to_cadwork matches point_to_rhino and conversely, there would be a. point_to_compas inside cadwork that defines the cadwork to compas conversion.
There was a problem hiding this comment.
Ahh, I see. In that case, we should put the conversions in compas_cadwork.conversions.
| @@ -0,0 +1,130 @@ | |||
| from abc import abstractmethod | |||
There was a problem hiding this comment.
I think this should be more top-level, not a 3-level nested utility
There was a problem hiding this comment.
The compas_cadwork.utils.storage is an internal helper module that provides the functionality needed for the Element.attributes, Element.attribute_names, Element.data, Project.attributes, Project.attribute_names, and Project.data properties. It is not meant to be used by end-users. In fact, we don't export it in "src/compas_cadwork/utils/__init__.py".
We can move it to "src/compas_cadwork/internal/storage.py" or to a different namespace if that helps.
| @@ -0,0 +1,6 @@ | |||
| from .ifc_uuid import IfcUUID | |||
There was a problem hiding this comment.
I generally don't like to have utils or utilities packages, because they tend to degrade into trash containers for everything that has not a better place in the library. In this case, we would be keeping two of them (one being 100% deprecated), why not keep at least the utilities container instead of creating utils?
There was a problem hiding this comment.
Fair point. I put the new modules in compas_cadwork.utils to make it clear for library users to know which modules are being deprecated, and because "utils" is fairly more common than "utilities" for Python projects.
Another option related to the previous comment would be to ditch the "utils" module and just put the class at compas_cadwork.ifc_uuid. Would that work?
| _K = TypeVar("_K") | ||
| _V = TypeVar("_V") |
There was a problem hiding this comment.
Neat generics-like syntax!
| from compas_cadwork.utils.converters import cwpoint_to_point | ||
| from compas_cadwork.utils.converters import cwpoint_to_vector | ||
| from compas_cadwork.utils.ifc_uuid import IfcUUID | ||
| from compas_cadwork.utils.storage import KeyValueStorage |
There was a problem hiding this comment.
Since the data model is based on this, why not make it more prominent and move to compas_cadwork.storage import ... or similar?
There was a problem hiding this comment.
This one I didn't understand, sorry 😅
Could you elaborate?
82329ea to
6316a12
Compare
3cd093a to
fad004c
Compare
Introduces new datamodels to represent Cadwork elements and the currently project, namely:
compas_cadwork.Project(new class)compas_cadwork.elements.Element(deprecatescompas_cadwork.datamodel.Element)It also creates the
compas_cadwork.IfcUUIDhelper class to represent IFC GUID, adds unit tests that mock the Cadwork API, and deprecates legacy classes and functions.