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 242 allow arbitrary filters but raise warning that we do not support them #451

Conversation

nikokaoja
Copy link
Collaborator

@nikokaoja nikokaoja commented May 14, 2024

[0.77.1] - 14-05-24

Added

  • Support for RawFilters allow arbitrary filters to be applied to the data model.

Copy link

github-actions bot commented May 14, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
20050 13649 68% 60% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cognite/neat/_version.py 100% 🟢
cognite/neat/rules/models/dms/_exporter.py 90% 🟢
cognite/neat/rules/models/dms/_rules.py 94% 🟢
cognite/neat/rules/models/wrapped_entities.py 83% 🟢
TOTAL 92% 🟢

updated for commit: 63218b8 by action🐍

@nikokaoja nikokaoja marked this pull request as ready for review May 14, 2024 10:40
@nikokaoja nikokaoja requested a review from a team as a code owner May 14, 2024 10:40
Copy link
Collaborator

@doctrino doctrino left a comment

Choose a reason for hiding this comment

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

A few improvements possible. Pre-approving, but suggest you do them. In particular, avoid introducing filter in the Base class.

@@ -15,6 +17,7 @@ class WrappedEntity(BaseModel, ABC):
name: ClassVar[str]
_inner_cls: ClassVar[type[Entity]]
inner: list[Entity] | None
filter: str | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be only used by the RawFilter, so is there any reason to have it on the base class?

Suggested change
filter: str | None = None

Comment on lines 94 to 97
if self.filter:
return self.filter
else:
return self.id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest just overriding the __repr__ in the RawFilter class

@@ -150,6 +192,7 @@ class HasDataFilter(DMSFilter):
name: ClassVar[str] = "hasData"
_inner_cls: ClassVar[type[ContainerEntity]] = ContainerEntity
inner: list[ContainerEntity] | None = None # type: ignore[assignment]
filter: str | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
filter: str | None = None

@@ -112,10 +129,35 @@ def from_dms_filter(cls, filter: dm.Filter) -> "DMSFilter":
if isinstance(entry, dict) and "space" in entry and "externalId" in entry
]
)
else:
return RawFilter(filter=json.dumps(dumped))

raise ValueError(f"Cannot convert {filter._filter_name} to {cls.__name__}")
Copy link
Collaborator

@doctrino doctrino May 14, 2024

Choose a reason for hiding this comment

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

This can now be removed as it cannot be reached. (The exception)


raise ValueError(f"Cannot convert {filter._filter_name} to {cls.__name__}")


class RawFilter(DMSFilter):
name: ClassVar[str] = "rawFilter"
inner: list[ContainerEntity] | None = None # type: ignore[assignment]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overwriting the base class forcing this to None

Suggested change
inner: list[ContainerEntity] | None = None # type: ignore[assignment]
inner: None = None # type: ignore[assignment]

@nikokaoja nikokaoja merged commit 28eb266 into main May 14, 2024
7 checks passed
@nikokaoja nikokaoja deleted the NEAT-242-Allow-arbitrary-filters-but-raise-warning-that-we-do-not-support-them branch May 14, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants