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-492] 🦟Core import bug #675

Merged
merged 26 commits into from
Oct 28, 2024
Merged

[NEAT-492] 🦟Core import bug #675

merged 26 commits into from
Oct 28, 2024

Conversation

doctrino
Copy link
Collaborator

@doctrino doctrino commented Oct 27, 2024

Before

image

After

image

Copy link

github-actions bot commented Oct 27, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
13747 9456 69% 60% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cognite/neat/_rules/models/dms/_exporter.py 94% 🟢
TOTAL 94% 🟢

updated for commit: 2d1a06b by action🐍

Copy link
Collaborator

@nikokaoja nikokaoja left a comment

Choose a reason for hiding this comment

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

See my comments on naming things

@@ -395,6 +400,43 @@ def _gather_properties(

return container_properties_by_id, view_properties_by_id

@staticmethod
def _gather_properties_with_parents(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest you move this under DMS Analysis module

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
def _gather_properties_with_parents(
def _gather_properties_with_ancestors(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Leverage cognite.neat._utils.rdf_.get_inheritance_path

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am using it in BaseAnalysis module under classes_with_properties where you can pass consider_inheritance flag

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The difference is that here you also set the child view as the view for all properties. Do you still think we should expose it in the DMSAnalysis or keep it as a private implementation detail?

@@ -120,7 +120,11 @@ def to_schema(self) -> DMSSchema:

containers = self._create_containers(container_properties_by_id, rules.enum) # type: ignore[arg-type]

views, view_node_type_filters = self._create_views_with_node_types(view_properties_by_id)
view_properties_with_parents_by_id = self._gather_properties_with_parents(view_properties_by_id, rules.views)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will return all the properties that are "inherited" from ancestors views , so maybe called this
view_ancestor_properties to make dist. between properties specifically defined for given view, and those coming from implements

@@ -198,6 +202,7 @@ def _create_spaces(
def _create_views_with_node_types(
self,
view_properties_by_id: dict[dm.ViewId, list[DMSProperty]],
view_properties_with_parents_by_id: dict[dm.ViewId, list[DMSProperty]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

see my naming suggestion

Copy link
Collaborator

@nikokaoja nikokaoja left a comment

Choose a reason for hiding this comment

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

See my comments on naming things and using util function for inheritance

Base automatically changed from core-import-bug to neat-core-extension October 28, 2024 10:34
Base automatically changed from neat-core-extension to main October 28, 2024 10:38
# Conflicts:
#	cognite/neat/_constants.py
#	cognite/neat/_rules/analysis/_dms.py
#	cognite/neat/_rules/transformers/_converters.py
#	cognite/neat/_session/_prepare.py
@@ -395,6 +400,43 @@ def _gather_properties(

return container_properties_by_id, view_properties_by_id

@staticmethod
def _gather_properties_with_parents(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am using it in BaseAnalysis module under classes_with_properties where you can pass consider_inheritance flag

@doctrino doctrino merged commit bf665b8 into main Oct 28, 2024
7 checks passed
@doctrino doctrino deleted the actual-bugfix branch October 28, 2024 11:21
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