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

fix: fix using wrong index in from_csv_row #15802

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Conversation

DerekMaggio
Copy link
Contributor

@DerekMaggio DerekMaggio commented Jul 25, 2024

Overview

I accidentally specified the same index multiple times when hardcoding from_csv_row.

Since hardcoding in this case is a no-no fix it by using built-in dataclass methods to look up headers and converting to/from csv.

Test Plan

  • Added unit tests that fail before fix was implemented due to hard coding
  • Added unit tests that ensure ordering of converted data. Although this tests the dataclass package more than it is testing our software, it is still helpful to have this because the docs aren't super clear about if the order is maintained

Changelog

  • Create a CSVStorageBase dataclass that contains shared methods
  • Have RawActivityData and ProcessResourceUsageSnapshot inherit from CSVStorageBase

Review requests

Is it weird having a blank dataclass as a base? I was having trouble when trying to use an ABC.

Risk assessment

The lowest of lows

@DerekMaggio DerekMaggio changed the title fix: fix using wrong index in from_csv_row permanently fix: fix using wrong index in from_csv_row Jul 25, 2024
@DerekMaggio DerekMaggio marked this pull request as ready for review July 30, 2024 16:36
@DerekMaggio DerekMaggio requested a review from a team as a code owner July 30, 2024 16:36
@DerekMaggio DerekMaggio requested a review from a team July 30, 2024 16:38
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

lgtm!

# Overview

Add dev docs and fix some docstrings
@DerekMaggio DerekMaggio requested a review from a team as a code owner July 31, 2024 19:48
@DerekMaggio DerekMaggio merged commit a477f16 into edge Jul 31, 2024
24 checks passed
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.

2 participants