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

Make association assets names more consistent #3035

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

bendnorman
Copy link
Member

PR Overview

This PR makes association assets more consistent by alphabetizing the linked sources and making sure linked sources come before linked entity names. See #3030 for more details.

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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bendnorman bendnorman linked an issue Nov 10, 2023 that may be closed by this pull request
@bendnorman
Copy link
Member Author

I decided to just recreate all of the migrations again because we're I ran into some migration conflicts and we're going to recreate them before we merge rename-core-asset into dev anyways.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (53d5618) 88.6% compared to head (1d2d71c) 88.6%.
Report is 8 commits behind head on rename-core-assets.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           rename-core-assets   #3035     +/-   ##
====================================================
- Coverage                88.6%   88.6%   -0.1%     
====================================================
  Files                      91      91             
  Lines                   11021   11017      -4     
====================================================
- Hits                     9773    9769      -4     
  Misses                   1248    1248             
Files Coverage Δ
src/pudl/analysis/classify_plants_ferc1.py 92.4% <ø> (ø)
src/pudl/analysis/eia_ferc1_record_linkage.py 96.1% <100.0%> (ø)
src/pudl/analysis/epacamd_eia.py 33.3% <ø> (ø)
src/pudl/analysis/plant_parts_eia.py 96.5% <ø> (ø)
src/pudl/etl/analysis_assets.py 85.7% <ø> (ø)
src/pudl/etl/epacems_assets.py 100.0% <ø> (ø)
src/pudl/etl/glue_assets.py 98.2% <100.0%> (ø)
src/pudl/extract/epacems.py 97.6% <ø> (ø)
src/pudl/extract/ferc1.py 99.1% <ø> (ø)
src/pudl/glue/ferc1_eia.py 98.5% <100.0%> (ø)
... and 19 more

☔ 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.

Only minor internal inconsistency I noticed was that now the epacamd_eia.py module is named differently from the reordered assets that it helps produce, and it seemed like having them named the same would be more intuitive.

Copy link
Member

Choose a reason for hiding this comment

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

For consistency, should we also ensure that the module names match the asset names? With the reordering, they no longer match.

@bendnorman
Copy link
Member Author

bendnorman commented Nov 15, 2023

Looks like there are a lot of instances of epacamd_eia being used for variables, module names, and the archive. How difficult would it be to rename the archive? I think it makes more sense to rename just the assets in this PR and do a deeper replacement of epacamd_eia with eia_epacamd in a separate PR once the archive has been renamed. We're planning on doing this with references to mcoe.

@zaneselvans zaneselvans self-requested a review November 15, 2023 20:35
@bendnorman bendnorman merged commit 578a033 into rename-core-assets Nov 15, 2023
10 of 11 checks passed
@bendnorman bendnorman deleted the rename-assn-assets branch November 15, 2023 20:46
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.

Iron out association asset names
2 participants