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

rewrite epiv2 exports #870

Merged
merged 16 commits into from Sep 27, 2023
Merged

rewrite epiv2 exports #870

merged 16 commits into from Sep 27, 2023

Conversation

dannypeterson
Copy link
Contributor

@dannypeterson dannypeterson commented Jul 31, 2023

Rewrite how the epiv2 export is calculated to combine multiple django ORM queries into a single request.

No changes were made to the export format of the data itself.

This PR created two new base classes, ModelExport, and Exporter, that we'll use throughout the application to update our modernize our existing export formats.

@dannypeterson dannypeterson marked this pull request as ready for review August 2, 2023 20:39
Copy link
Collaborator

@rabstejnek rabstejnek left a comment

Choose a reason for hiding this comment

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

Looking good! There were some commented TODOs left in here that I think should be addressed before merge, which makes up the bulk of the comments I left. Also once those are wrapped up you should regenerate the VCR fixture for the epi api test so that it passes.

hawc/apps/epiv2/api.py Outdated Show resolved Hide resolved
hawc/apps/epiv2/managers.py Outdated Show resolved Hide resolved
hawc/apps/epiv2/managers.py Outdated Show resolved Hide resolved
Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

Ok, you've convinced me, this is a nice pattern! Some feedback on the API and layout

hawc/apps/common/exports.py Outdated Show resolved Hide resolved
hawc/apps/common/exports.py Outdated Show resolved Hide resolved
hawc/apps/common/exports.py Outdated Show resolved Hide resolved
hawc/apps/common/models.py Outdated Show resolved Hide resolved
hawc/apps/study/exports.py Outdated Show resolved Hide resolved
hawc/apps/common/exports.py Show resolved Hide resolved
Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

LGTM! I made two minor changes, if you're ok with these. I figure in the future we'll get rid of the FlatFileExporter and just use this Export class directly; you can see I was able to do so in the epiv2 api (but couldn't get rid of it b/c the DataPivotQuery still uses the old one).

  • 0c2d975 add special comment (17 seconds ago) {Andy Shapiro}
  • 8f2abed updates (11 minutes ago) {Andy Shapiro}

@rabstejnek rabstejnek merged commit 1f9ad6c into main Sep 27, 2023
3 checks passed
@rabstejnek rabstejnek deleted the rewrite-epiv2-exports branch September 27, 2023 17:58
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.

None yet

3 participants