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.

Sign up for GitHub

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

Jump to bottom

Feat: 5530 feature updated at inserted at properties on retrieved records #5540

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Feat: 5530 feature updated at inserted at properties on retrieved records #5540

wants to merge 13 commits into from

Conversation

davidberenstein1957
Copy link
Member

Description

Closes #5530

Type of change

  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

Checklist

  • I added relevant documentation
  • I followed the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • I confirm My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@davidberenstein1957 davidberenstein1957 linked an issue Sep 25, 2024 that may be closed by this pull request
[FEATURE] updated_at / inserted_at properties on retrieved Records #5530
Open
@davidberenstein1957 davidberenstein1957 changed the title Feat/5530 feature updated at inserted at properties on retrieved records Feat: 5530 feature updated at inserted at properties on retrieved records Sep 25, 2024
@davidberenstein1957 davidberenstein1957 marked this pull request as ready for review September 25, 2024 15:29
@davidberenstein1957
Copy link
Member Author

@frascuchon do you think it also makes sense to update the __repr__ of all resources to include this info?

@frascuchon
Copy link
Member

@frascuchon do you think it also makes sense to update the __repr__ of all resources to include this info?

Yes, I think it makes sense

frascuchon
frascuchon reviewed Sep 26, 2024
View reviewed changes
argilla/tests/integration/test_list_records.py
@@ -106,3 +106,21 @@ def test_list_records_with_responses(client: Argilla, dataset: Dataset):

assert records[1].responses["comment"][0].value == "The comment"
assert records[1].responses["sentiment"][0].value == "negative"


def test_list_records_with_updated_at_and_inserted_at(client: Argilla, dataset: Dataset):
Copy link
Member

Choose a reason for hiding this comment

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

Should we expose these new attributes in the to_dict methods? cc @burtenshaw

Copy link
Member Author

Choose a reason for hiding this comment

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

for me it makes sense too.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be ok. They will be used in records._io._generic and exported in all formats, so we should validate serialisation.

Copy link
Member

Choose a reason for hiding this comment

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

I see the .serialize methods already serialize these new attributes. I would leave it as is and review the logic when unifying to_dict and serialize methods: Ref #4944

(I added a todo commit 87cb1eb)

@codecov Codecov
Copy link

codecov bot commented Sep 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.21%. Comparing base (d57a83b) to head (87cb1eb).

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #5540   +/-   ##
========================================
  Coverage    91.21%   91.21%           
========================================
  Files          145      145           
  Lines         5964     5964           
========================================
  Hits          5440     5440           
  Misses         524      524
Flag Coverage Δ
argilla-server 91.21% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidberenstein1957 davidberenstein1957 force-pushed the feat/5530-feature-updated_at-inserted_at-properties-on-retrieved-records branch from 4338d83 to 45db576 Compare September 27, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frascuchon frascuchon frascuchon left review comments

@burtenshaw burtenshaw burtenshaw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] updated_at / inserted_at properties on retrieved Records
3 participants
@davidberenstein1957 @frascuchon @burtenshaw