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

Fix data modeling data type Enum dumping #2091

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Fix data modeling data type Enum dumping #2091

merged 4 commits into from
Jan 15, 2025

Conversation

haakonvt
Copy link
Contributor

@haakonvt haakonvt commented Jan 14, 2025

@haakonvt haakonvt requested review from a team as code owners January 14, 2025 14:17
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.68%. Comparing base (0e564df) to head (42190b2).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2091   +/-   ##
=======================================
  Coverage   90.67%   90.68%           
=======================================
  Files         149      149           
  Lines       22881    22880    -1     
=======================================
  Hits        20748    20748           
+ Misses       2133     2132    -1     
Files with missing lines Coverage Δ
cognite/client/_api/ai/tools/documents.py 78.57% <100.00%> (+7.98%) ⬆️
cognite/client/_api_client.py 90.34% <ø> (ø)
cognite/client/_version.py 100.00% <100.00%> (ø)
...te/client/data_classes/data_modeling/containers.py 98.83% <100.00%> (ø)
...te/client/data_classes/data_modeling/data_types.py 97.17% <100.00%> (+0.08%) ⬆️

... and 2 files with indirect coverage changes

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.

A few clarifying questions, otherwise all good.

Comment on lines +51 to +52
res = self._post(self._RESOURCE_PATH + "/summarize", json={"items": ident.as_dicts()})
return Summary._load(res.json()["items"][0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes #42190b2

@@ -276,8 +276,8 @@ def dump(self, camel_case: bool = True) -> dict[str, str | dict]:
output["immutable"] = self.immutable
for key in ["nullable", "auto_increment", "name", "default_value", "description"]:
if (value := getattr(self, key)) is not None:
output[key] = value
return convert_all_keys_to_camel_case_recursive(output) if camel_case else output
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a dangerous function. Should we have a task to remove it? Or do it have any good use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I had the same thought!

Comment on lines +140 to +143
def test_ai(self):
run_docstring_tests(ai)
run_docstring_tests(ai.tools)
run_docstring_tests(ai.tools.documents)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have they forgotten to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They did, and I missed it in review

@haakonvt haakonvt merged commit 8fc4208 into master Jan 15, 2025
16 checks passed
@haakonvt haakonvt deleted the fix-enum-dumping branch January 15, 2025 08:03
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