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

Rename heat_rate_mmbtu_mwh -> unit_heat_rate_mmbtu_per_mwh... again #3029

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

bendnorman
Copy link
Member

@bendnorman bendnorman commented Nov 8, 2023

PR Overview

We renamed the heat_rate_mmbtu_mwh column to unit_heat_rate_mmbtu_per_mwh but merged the changes into a branch that didn't get merged in the main feature branch rename-core-assets. This PR rebases the changes from #2917 onto rename-core-assets.

PR Checklist

  • Merge the most recent version of the branch you are merging into (probably dev).
  • All CI checks are passing. Run tests locally to debug failures
  • Make sure you've included good docstrings.
  • For major data coverage & analysis changes, run data validation tests
  • Include unit tests for new functions and classes.
  • Defensive data quality/sanity checks in analyses & data processing functions.
  • Update the release notes and reference reference the PR and related issues.
  • Do your own explanatory review of the PR to help the reviewer understand what's going on and identify issues preemptively.

@bendnorman bendnorman requested review from zaneselvans and removed request for zaneselvans November 8, 2023 01:53
@bendnorman bendnorman marked this pull request as draft November 8, 2023 01:56
@bendnorman bendnorman changed the title Rename heat_rate_mmbtu_mwh -> heat_rate_mmbtu_mwh_by_unit... again Rename heat_rate_mmbtu_mwh -> unit_heat_rate_mmbtu_per_mwh... again Nov 8, 2023
@bendnorman bendnorman marked this pull request as ready for review November 8, 2023 22:04
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0c3b9ae) 88.6% compared to head (0f90efa) 88.6%.
Report is 84 commits behind head on rename-core-assets.

Additional details and impacted files
@@                Coverage Diff                 @@
##           rename-core-assets   #3029   +/-   ##
==================================================
  Coverage                88.6%   88.6%           
==================================================
  Files                      91      91           
  Lines                   11019   11021    +2     
==================================================
+ Hits                     9771    9773    +2     
  Misses                   1248    1248           

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

Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

The only question I have is about the alembic migrations that have been changed. Should they really all be getting changed piecemeal like this? Or should the old ones keep referring to the old column name since... old DBs will have the old column name.

Or should we just wipe the migrations out and start anew?

Copy link
Member

Choose a reason for hiding this comment

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

What's up with there being several migration versions in here changing? Including older ones? Is this search-and-replace gone wild? It seems like the older migrations might actually need to refer to the older column name.

Alternatively, should we wipe all the migrations periodically to keep too many layers from building up? Since I don't think we actually intend for it to be possible to migrate between all database versions.

@bendnorman
Copy link
Member Author

I've occasionally been clobbering and recreating the migrations when branches off of rename-core-assets start to diverge. I figured this was ok because we'll probably clobber and recreate a revision once we merge rename-core-assets into dev.

I don't think there is a ton of value keeping all of these small revision changes. Because we're not maintaining the schema of a production database, migrations are mostly helpful on single branches when you adjusting schemas and don't want to rerun the entire ETL to test your changes.

@zaneselvans
Copy link
Member

My concern is that it seems like there's a pre-existing auto-generated migration that's being edited here to use the new column name, which seems a bit odd -- shouldn't the old migration use the old column name?

@bendnorman
Copy link
Member Author

Good catch! I must have been a little too trigger-happy and accidentally renamed the columns in old revision files.

I deleted 1ceb and 3313 because they are from dev and couldn't be easily merged with the "base" revision on the rename-core-assets branch. The changes in those two revisions are captured in 9ccf.

@zaneselvans zaneselvans self-requested a review November 15, 2023 20:30
@bendnorman bendnorman merged commit 1396ad8 into rename-core-assets Nov 16, 2023
9 checks passed
@bendnorman bendnorman deleted the rename-heat-rate-mmbtu-mwh-column branch November 16, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants