-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
LB-1419: Fix broken partial dates #3029
base: master
Are you sure you want to change the base?
Conversation
Hello @mayhem! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
This PR is also temporarily deployed, anticipating the monthly rebuild of all of the data. |
@@ -133,9 +133,16 @@ def create_json_data(self, row): | |||
tag["genre_mbid"] = genre_mbid | |||
release_group_tags.append(tag) | |||
|
|||
|
|||
date = str(row["year"] or '') | |||
if row["month"] is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same format that MB follows for partial dates because consider a date where the month is unknown but the year and day are, it could be formatted as 2024-01 which is easily misunderstood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MB dates are not allowed to have a day without a month. YYYY, YYYY-MM, YYYY-MM-DD are the only valid forms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah cool, awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we are relying on the date to be parsable anywhere but in case something breaks we should keep this change in mind. Also, you could use COALESCE in postgres to generate the partial dates but python is fine too.
The MB data cashes have invalid partial dates for release_group data. The problem is that when any part of a || concat expression in PG is NULL, the entire result is NULL. The solution is to move the partial data logic into python code.