Skip to content

feat: limit analytics package to static site reports and remove unnecessary requirements (#4912) - #4926

Merged
NoopDog merged 11 commits into
mainfrom
hunter/4912-analytics-package-static-site-only
Aug 15, 2026
Merged

NoopDog merged 11 commits into
mainfrom
hunter/4912-analytics-package-static-site-only

Conversation

@hunterckx

Copy link
Copy Markdown
Contributor

Closes #4912

  • Removed notebook- and sheets-specific code
  • Renamed sheets_elements.py and _sheets_utils.py to report_elements.py and _report_utils.py
  • Moved get_data_df and get_df_over_time from charts.py to _report_utils.py, and deleted the rest of charts.py (which was unused and related to the Jupyter Book reports)
  • Updated analytics package requirements and version, and updated requirements.txt based on a fresh install
  • Updated a data type check that's meant to include string columns to work with new Pandas 3 data type inference

Note: Some unused definitions related to the sheets reports are left in (such as API definitions and functions for creating dataframes for certain types of reports), but none have any unique dependencies

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR slims the analytics package down to the static-site reporting core by removing legacy notebook/Sheets dependencies and relocating GA fetch helpers to a report-focused utility module.

Changes:

  • Removed legacy Google Sheets and notebook/charting code paths (including dropping gspread/matplotlib-related modules).
  • Renamed Sheets-oriented modules to report-oriented modules and updated static-site imports accordingly.
  • Updated packaging/versioning and refreshed analytics/requirements.txt; adjusted Pandas dtype handling for export.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
analytics/requirements.txt Regenerated/minimized pinned environment deps for the static-site-focused analytics workflow.
analytics/analytics_package/setup.py Bumped major version and removed Sheets/notebook dependencies from install_requires.
analytics/analytics_package/analytics/static_site/fetch.py Updated imports to use report_elements and _report_utils.
analytics/analytics_package/analytics/static_site/export.py Updated dtype handling when exporting DataFrames to JSON.
analytics/analytics_package/analytics/sheets_api.py Deleted legacy gspread-based Sheets API implementation.
analytics/analytics_package/analytics/report_elements.py Removed Sheets-only formatting constants and switched to _report_utils.
analytics/analytics_package/analytics/charts.py Deleted unused notebook/matplotlib-based charting module after extracting needed helpers.
analytics/analytics_package/analytics/_report_utils.py Inlined GA fetch helpers (get_data_df, get_df_over_time) previously sourced from charts.py.
Suppressed comments (1)

analytics/analytics_package/analytics/_report_utils.py:12

  • The newly added helpers (get_data_df / get_df_over_time) are indented with hard tabs, while the rest of this module uses 4-space indentation. Mixing indentation styles within one file is error-prone (especially if future edits introduce mixed whitespace within the same block) and makes diffs noisier; please reindent these new functions to match the module’s existing style.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread analytics/analytics_package/analytics/static_site/export.py
Comment thread analytics/analytics_package/setup.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

analytics/analytics_package/analytics/_report_utils.py:12

  • The newly added helper functions (get_data_df, strings_to_lists, get_df_over_time) are indented with tab characters, while the rest of this module uses 4-space indentation. Mixing tabs and spaces in the same file is error-prone and can trigger lint/format failures (and potentially TabError if mixed within a block). Please reindent these new functions to match the module’s 4-space style.

@NoopDog NoopDog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@hunterckx thx — here's the review. Five inline comments below; three findings land on lines outside the diff hunks, so noting them here. Several of these were verified empirically in a venv with the pinned pandas 3.0.5.

analytics/analytics_package/analytics/report_elements.py:36 (the most important finding) — pd.set_option('future.no_silent_downcasting', True) is deprecated and a no-op under the pandas 3.0.5 this PR pins: verified that every get_outbound_links_df call (i.e. every report generation for all four sites) emits Pandas4Warning, and when pandas 4 lands set_option will raise OptionError, crashing static-site generation at the outbound-links step (on pandas <2.2 it raises OptionError today). The pandas-3 migration should delete this line.

analytics/analytics_package/analytics/report_elements.py:256 — ~300 lines of zero-caller sheets-report code remain (get_landing_page_df/_change, get_index_table_download_change, the get_index_entity_selected/sorted/paginated df/_change pairs, get_event_count_over_time_df — which strands get_change_over_time_df_multiple_events in _report_utils.py:157 — plus unreferenced yt/drive/sheets service params and OAuth scopes in api.py:14-33). Verified branch-wide via git grep: every listed symbol has zero live callers. The PR description discloses this deferral, so non-blocking — just flagging the concrete list for the follow-up.

analytics/analytics_package/analytics/_report_utils.py:128 — get_change_over_time_df passes format_table=False, but format_table was a parameter of the deleted charts.py show_plot_over_time and is now a dead kwarg silently swallowed by the **other_params catch-alls three levels deep (verified it never reaches the GA request body — both v3/v4 request dicts are built from explicit keys, and no consumer exists anywhere on the branch). Since this PR deleted its only consumer, dropping the kwarg belongs here.

Comment thread analytics/analytics_package/setup.py Outdated
Comment thread analytics/analytics_package/analytics/static_site/export.py
Comment thread analytics/analytics_package/analytics/_report_utils.py
Comment thread analytics/analytics_package/analytics/_report_utils.py
Comment thread analytics/analytics_package/analytics/report_elements.py

@NoopDog NoopDog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed after the fix commits — the confirmed issues are all addressed: deprecated no_silent_downcasting option removed, pandas version constrained in package metadata, dead format_table kwarg dropped, indentation normalized. Remaining review items are out of scope for this PR: the star-import cleanup is ticketed as #4927 (linked from #4913), and the rest (export coercion design, latent get_df_over_time guard, pd.to_numeric simplification, disclosed dead code) are latent/pre-existing and non-blocking. LGTM — thanks @hunterckx!

@NoopDog

NoopDog commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Summary of review wrap-up:

  • Approved — the fix commits address all confirmed findings from the review: removed the deprecated future.no_silent_downcasting option, constrained the pandas version in setup.py, dropped the dead format_table kwarg, and normalized indentation.
  • Resolved all open review threads — the remaining items were agreed out of scope for this PR.
  • Spun out chore: make analytics package public exports explicit (remove star-import leak) #4927 for the star-import cleanup (explicit exports / __all__, plus making fetch.py's ADDITIONAL_DATA_BEHAVIOR import explicit) — assigned to @hunterckx, added to the CC All Projects board, and linked from the chore: retire legacy analytics formats — tracking #4913 tracking issue.
  • Not ticketed (latent/pre-existing, non-blocking): the export.py dtype-sniffing vs col_map design point, the latent get_df_over_time xlabels guard, the pd.to_numeric simplification, and the disclosed dead sheets code.

@NoopDog
NoopDog merged commit e49d6f6 into main Aug 15, 2026
3 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.

chore: slim analytics package to the static-site core

3 participants