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

[NEAT-108] Transformation/RawTable/RawDatabase Loaders #316

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

doctrino
Copy link
Collaborator

@doctrino doctrino commented Mar 16, 2024

Preparation for exporting Raw Tables and Transformations.

Extended cdf_loaders with TransformationLoader, RawDatabaseLoader and RawTableLoader.

The module started to become big so I split it into a package for better developer experience. Had to take special care of the RawTableLoader, documented reason in code.

@@ -0,0 +1,25 @@
from ._base import ResourceLoader
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this at some point migrate under a special type of loaders under rules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Loader is a bit overloaded here, loaders here are not the same as in graph.


from .data_classes import RawTableID

T_ID = TypeVar("T_ID", bound=str | int | DataModelingId | InstanceId | VersionedDataModelingId | RawTableID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about some more readable name .e.g. T_CogniteResourceId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great, will include.


@classmethod
@abstractmethod
def get_id(cls, item: T_WriteClass | T_WritableCogniteResource) -> T_ID:
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion maybe rename this to as_id ? wandering do you need two separate methods one for listable, one for single items? can there be a single as_id method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

even better call this as_external_ids to comply with sdk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I want to distinguish this from .as_id(), .as_ids() which are on most Cognite Resource objects.

WriteableCogniteResourceList,
)

# The Table, TableWrite data classes in the Cognite-SDK lacks the database attribute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this eventually find its way to cognite-sdk ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, hopefully, but the problem is that would be a breaking change (to the poor RAW implementation), so it would be breaking the SDK.

@doctrino doctrino merged commit c724463 into main Mar 18, 2024
6 checks passed
@doctrino doctrino deleted the export-transformations-part1 branch March 18, 2024 13:22
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