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

add last_updated_time_in_index to retrieve_dataframe #1846

Merged
merged 16 commits into from
Jul 26, 2024

Conversation

KristianVangsnes
Copy link
Contributor

@KristianVangsnes KristianVangsnes commented Jul 11, 2024

Description

Adding last_updated_time_in_index to retrieve_dataframe function, which makes index into multi-index. Also updated docstring.

Checklist:

  • Tests added/updated.
  • Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
    If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.
  • Changelog updated in CHANGELOG.md.
  • Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

…ch makes index into multi-index. Also updated docstring
@KristianVangsnes KristianVangsnes requested review from a team as code owners July 11, 2024 11:59
@KristianVangsnes KristianVangsnes requested review from mattmurph9 and removed request for a team July 11, 2024 11:59
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.06%. Comparing base (91c82e6) to head (6d17c01).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1846   +/-   ##
=======================================
  Coverage   91.05%   91.06%           
=======================================
  Files         124      124           
  Lines       19245    19247    +2     
=======================================
+ Hits        17524    17527    +3     
+ Misses       1721     1720    -1     
Files Coverage Δ
cognite/client/_api/raw.py 89.12% <100.00%> (+0.09%) ⬆️
cognite/client/_version.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

LGTM! Let's add a test making sure it works when len(rows) is 0, 1, and many 👌

@KristianVangsnes
Copy link
Contributor Author

Where would be best to add the test? tests_unit/test_api/test_raw.py?

@haakonvt
Copy link
Contributor

Where would be best to add the test? tests_unit/test_api/test_raw.py?

Yup!

Comment on lines 759 to 761
idx = pd.MultiIndex.from_tuples(
[(r.key, r.last_updated_time) for r in rows], names=["key", "last_updated_time"]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we convert epoch-ms to datetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions, I don't see why not.

Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

LGTM! Might need to update one or more tests after making it a timestamp? Also, please try to revert the unrelated docstring changes (unless you actually fixed something in them 👌 )

@haakonvt haakonvt changed the title RAW: Adding last_updated_time_in_index to retrieve_dataframe function, whi… add last_updated_time_in_index to retrieve_dataframe Jul 26, 2024
@KristianVangsnes KristianVangsnes merged commit a661630 into master Jul 26, 2024
14 checks passed
@KristianVangsnes KristianVangsnes deleted the raw/add-option-last-updated-time-in-index branch July 26, 2024 12:32
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