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

Serialisation aliases #56

Merged
merged 10 commits into from
Jul 4, 2024
Merged

Serialisation aliases #56

merged 10 commits into from
Jul 4, 2024

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Jul 1, 2024

Companion PR to https://github.com/Urban-Analytics-Technology-Platform/popgetter/pull/130.

Merge that first and regenerate all the BE/NI data before testing this PR! The CI is failing now because the proposed changes do not match our current data.

This PR:

  • Reworks the function which joins all metadata tables to use the new column names.
  • Defines all the column names in a single struct impl, and gets rid of all the remaining magic constants.

It would be cool if we had a test to get the column names from upstream Python and check that they are equal to the ones here. I haven't done that, though. Alternatively, if we one day merge the two repos into one, they could potentially be derived from the same file.

@penelopeysm
Copy link
Member Author

penelopeysm commented Jul 2, 2024

The tests don't currently pass. However, they should (hopefully!) pass if you do either one of these:

  • Regenerate all the upstream data (now that the previous PRs have been merged) and place it in the popgetter-dagster-test Azure container
  • Point the config base path instead to https://popgetter.blob.core.windows.net/prod/0.1.0, where I've uploaded a subset of the Belgium and NI data (enough to make the tests pass, anyway).

src/lib.rs Outdated Show resolved Hide resolved
src/display.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@sgreenbury
Copy link
Contributor

Thanks for this @penelopeysm, looks great!

I've just updated the config b131f59 to point to a path with the new data (will open an issue to point to modify to point to same path as the final released data) and added a few small comments/queries above in the code.

@sgreenbury sgreenbury mentioned this pull request Jul 4, 2024
4 tasks
@sgreenbury
Copy link
Contributor

Merging this now, thanks @penelopeysm!

@sgreenbury sgreenbury merged commit a8c715d into main Jul 4, 2024
2 checks passed
@sgreenbury sgreenbury deleted the serialisation-aliases branch July 4, 2024 16:34
@sgreenbury sgreenbury mentioned this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done:
Development

Successfully merging this pull request may close these issues.

2 participants