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

WIP: take CDM/v1 using typed instances to GA #1919

Merged
merged 12 commits into from
Sep 16, 2024
Merged

Conversation

haakonvt
Copy link
Contributor

@haakonvt haakonvt commented Sep 9, 2024

Description

This PR takes CDM/v1 using typed instances to GA.

DM-2068

@haakonvt haakonvt requested review from a team as code owners September 9, 2024 21:52
@haakonvt haakonvt requested review from amorken and removed request for a team September 9, 2024 21:52
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 89.79592% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.28%. Comparing base (dd79d3c) to head (11b6c45).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
cognite/client/_api/data_modeling/instances.py 40.00% 3 Missing ⚠️
...ite/client/data_classes/data_modeling/instances.py 95.23% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1919   +/-   ##
=======================================
  Coverage   89.28%   89.28%           
=======================================
  Files         131      131           
  Lines       20806    20808    +2     
=======================================
+ Hits        18576    18579    +3     
+ Misses       2230     2229    -1     
Files with missing lines Coverage Δ
cognite/client/_api/data_modeling/views.py 95.74% <ø> (ø)
...te/client/data_classes/data_modeling/data_types.py 97.09% <100.00%> (ø)
...ite/client/data_classes/data_modeling/instances.py 90.97% <95.23%> (-0.13%) ⬇️
cognite/client/_api/data_modeling/instances.py 87.59% <40.00%> (+0.06%) ⬆️

... and 2 files with indirect coverage changes

@haakonvt haakonvt changed the title take CDM/v1 using typed instances to GA WIP: take CDM/v1 using typed instances to GA Sep 10, 2024
@erlendvollset
Copy link
Collaborator

What's the deal with the use_attribute_name parameter on TypedInstance.dump? It dumps the custom properties using the attribute names in the defined class? That doesn't seem valuable to me and it's adding a bunch of complexity. I say we remove that.

Copy link
Contributor

@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.

Main comment is to avoid manually editing the generated files, and instead update the source (pygen) template

@erlendvollset
Copy link
Collaborator

So per now TypedNode(..).properties returns an empty dict, and TypedNodeApply().sources returns None. They’re just being resolved correctly when dumping, which is not how it should be.

@erlendvollset
Copy link
Collaborator

CogniteDescribableNode/CogniteSchedulableNode etc. need to be renamed. Those can be used for edges as well as nodes.

@haakonvt
Copy link
Contributor Author

@erlendvollset / @doctrino PR is reverted to not include any changes to datetime and TypedNode inheritance changes.

@doctrino doctrino self-requested a review September 12, 2024 08:50
Copy link
Contributor

@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.

Just add back the warning and then good to go.

@haakonvt haakonvt requested a review from doctrino September 12, 2024 09:06
@haakonvt haakonvt force-pushed the typed-instances-ga branch 2 times, most recently from 7b61d12 to 838164b Compare September 14, 2024 22:43
@haakonvt haakonvt enabled auto-merge (squash) September 16, 2024 14:00
@haakonvt haakonvt merged commit 327467f into master Sep 16, 2024
14 checks passed
@haakonvt haakonvt deleted the typed-instances-ga branch September 16, 2024 14:01
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.

3 participants