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.

Sign up for GitHub

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

Update fix/clean up #4068

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Update fix/clean up #4068

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a2ac8d
Just some cleanup
NicholasTurner23 Dec 13, 2024
44abcbc
Merge branch 'staging' into update-fix/Clean_up
NicholasTurner23 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/analytics/api/views/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,14 @@ def post(self):
)
if minimum_output:
# Drop unnecessary columns
columns_to_drop = ["site_id"]
columns_to_drop.append("timestamp") if frequency in [
"hourly",
"daily",
] else columns_to_drop
print(data_frame.columns.to_list())
data_frame.drop(
columns=[
"site_id",
"timestamp",
],
columns=columns_to_drop,
Comment on lines +166 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove debugging print statement and improve code readability

A few suggestions for improvement:

  1. The print statement appears to be debugging code that should be removed or replaced with proper logging
  2. The conditional append logic could be more readable
  3. Consider validating column existence before dropping

Here's a suggested improvement:

-                columns_to_drop = ["site_id"]
-                columns_to_drop.append("timestamp") if frequency in [
-                    "hourly",
-                    "daily",
-                ] else columns_to_drop
-                print(data_frame.columns.to_list())
-                data_frame.drop(
-                    columns=columns_to_drop,
-                    inplace=True,
-                )
+                columns_to_drop = ["site_id"]
+                if frequency in ["hourly", "daily"]:
+                    columns_to_drop.append("timestamp")
+                
+                # Only drop columns that exist
+                existing_columns = [col for col in columns_to_drop if col in data_frame.columns]
+                if existing_columns:
+                    logger.debug(f"Dropping columns: {existing_columns}")
+                    data_frame.drop(columns=existing_columns, inplace=True)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
columns_to_drop = ["site_id"]
columns_to_drop.append("timestamp") if frequency in [
"hourly",
"daily",
] else columns_to_drop
print(data_frame.columns.to_list())
data_frame.drop(
columns=[
"site_id",
"timestamp",
],
columns=columns_to_drop,
columns_to_drop = ["site_id"]
if frequency in ["hourly", "daily"]:
columns_to_drop.append("timestamp")
# Only drop columns that exist
existing_columns = [col for col in columns_to_drop if col in data_frame.columns]
if existing_columns:
logger.debug(f"Dropping columns: {existing_columns}")
data_frame.drop(columns=existing_columns, inplace=True)

inplace=True,
)

Expand Down
Loading