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

refactor: rename timestamps #1110

Merged
merged 2 commits into from
Nov 28, 2024
Merged

refactor: rename timestamps #1110

merged 2 commits into from
Nov 28, 2024

Conversation

MatMoore
Copy link
Contributor

@MatMoore MatMoore commented Nov 27, 2024

There are various datetimes returned by Datahub, and it's easy to confuse them. It seems like we are not always using the right one, so as a first step I'm renaming all the fields and parse functions to clearly describe what they are actually returning.

I've also exposed Datahub's lastModified value as I think this is the one we should be using for data last updated.

A timestamp documenting when the asset was last modified in the source Data Platform (not on DataHub)

This does not change what is displayed yet, I'll do that in a follow up PR.

There are various datetimes returned by Datahub, and it's easy to
confuse them. It seems like we are not always using the right one, so as
a first step I'm renaming all the parse functions to clearly describe
what they are actually returning.
@MatMoore MatMoore requested a review from a team as a code owner November 27, 2024 15:35
Copy link
Contributor

@LavMatt LavMatt left a comment

Choose a reason for hiding this comment

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

Do you think we could go a bit further and rename the properties in the pydantic entity model. Every entity should(or could) have a data_last_modified and a metadata_last_updated property so we could put these in the base entity class and remove reference to them elsewhere in entities.py? Plus they'd be clearer when used in the find-moj-data code

@MatMoore
Copy link
Contributor Author

Do you think we could go a bit further and rename the properties in the pydantic entity model. Every entity should(or could) have a data_last_modified and a metadata_last_updated property so we could put these in the base entity class and remove reference to them elsewhere in entities.py? Plus they'd be clearer when used in the find-moj-data code

Yeah that makes sense... I'll have a look at this now

We have several timestamps that are easily confused. To try and make the
code clearer, I'm defining them for all entities with explicit names
that correspond to how they are stored in Datahub.
@MatMoore MatMoore changed the title refactor: rename timestamp parse functions refactor: rename timestamps Nov 28, 2024
@MatMoore MatMoore merged commit 75d2ce5 into main Nov 28, 2024
22 of 23 checks passed
@MatMoore MatMoore deleted the rename-timestamps branch November 28, 2024 14:07
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