Repository navigation
Add historic package lists - #133
andrewnaessens wants to merge 27 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yselkowitz
left a comment
There was a problem hiding this comment.
There seems to be some boilerplate-like code here, is there an opportunity to create some functions which can be reused in multiple places?
| history_data = {} | ||
| history_data["date"] = str(now.strftime("%Y-%m-%d")) | ||
| history_data["workloads"] = {} | ||
| history_data["envs"] = {} | ||
| history_data["repos"] = {} | ||
| history_data["views"] = {} |
There was a problem hiding this comment.
IIUC we've settled on an initialization syntax for these.
There was a problem hiding this comment.
Updated to dict literal syntax in save_current_historic_data_daily.
| # Generate daily Chart.js JSON — same chart types as _generate_chartjs_data | ||
| # but reading from daily snapshots instead of weekly |
There was a problem hiding this comment.
Do we need both? Or can we just use daily throughout?
There was a problem hiding this comment.
Removed _generate_chartjs_data_daily and chart toggle. Chart is now generated from weekly snapshots for previous historic data and daily snapshots going forward.
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/flatpickr/dist/flatpickr.min.css"> | ||
| <script src="https://cdn.jsdelivr.net/npm/flatpickr"></script> |
There was a problem hiding this comment.
This should be like the others, with a version-specific src and integrity, and also make sure it's covered by the update checking workflow.
There was a problem hiding this comment.
Pinned to v4.6.13 with SRI and added to check_cdn_updates.py.
|
Also, could the exisitng weekly data be of any use to "backfill" the daily data? |
Extracted |
Existing weekly counts are displayed on the chart alongside new daily counts. Package lists are not captured in existing weekly snapshots so browse/compare feature will only contain package lists from the point it is introduced. |
bhoy-troy
left a comment
There was a problem hiding this comment.
Looks good, just a couple of minor things to improve it.
|
|
||
| # Sorted for consistent output | ||
| view_pkgs = { | ||
| "all_source_names": sorted(list(all_source_names)), |
There was a problem hiding this comment.
No need to use list(), sorted already returns a list.
There are a number of instances of this, and others that have been there long before we arrived 😃
| filename = f"historic_data_pkgs-{year}-day_{day_of_year}.json" | ||
| output_dir = os.path.join(query.settings["output"], "history") | ||
| os.makedirs(output_dir, exist_ok=True) | ||
| file_path = os.path.join(output_dir, filename) |
There was a problem hiding this comment.
This block is duplicated in _save_current_historic_data_daily and _save_current_historic_data_daily_pkgs. There are also other modules that use similar style, that have been there a long time.
Can this be put into a function that can be reused to reduce duplication?
We can update the other modules at a later date when the need arises.
There was a problem hiding this comment.
Extracted helper function _daily_snapshot_file_path to reduce duplication.
|
|
||
| merged = {} | ||
|
|
||
| for entry in historic_data_weekly.values(): |
There was a problem hiding this comment.
could be replaced with a dict comprehension
There was a problem hiding this comment.
Replaced with dict comprehension.
| # Filter for daily files only (not weekly) | ||
| all_filenames = os.listdir(directory) | ||
| valid_filenames = [] | ||
| for filename in all_filenames: |
There was a problem hiding this comment.
Could use a more Pythonic approach with a comprehension.
valid_filenames = sorted(
f for f in os.listdir(directory)
if re.match()
)
There was a problem hiding this comment.
Used suggested pythonic approach.
| all_filenames = os.listdir(directory) | ||
| valid_filenames = [] | ||
| for filename in all_filenames: | ||
| if bool(re.match(r"historic_data-....-day_....json", filename)): |
There was a problem hiding this comment.
the . in .json isn't escaped; it works as it's just interpreted by the regex as any char
Also consider using something like r"historic_data-\d{4}-day_\d{3}\.json" if you expect the characters to be digits only.
There was a problem hiding this comment.
Improved regex:
_read_historic_data - r"historic_data-\d{4}-week_\d{2}\.json"
_read_historic_data_daily - r"historic_data-\d{4}-day_\d{3}\.json"
Add daily package list snapshots with browse and compare UI features
Add daily snapshots of each view's package lists and counts. Package lists are viewable from chart data points and with browse and compare calendar date pickers.
Resolves #25
Summary
generate_historic_data()beforegenerate_pages()so date list is available to templates