Add page for downloading .ics files#3282
Conversation
pareshjoshij
requested review from
KJhellico,
PPsyrius and
arkid15r
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new site asset generator script and a language registry, ignores generated downloads in Git, and adds a lint-ignore for the new script. The generator produces ICS and per-year inline JSON assets plus per-entity manifests and a top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_site_assets.py`:
- Around line 127-128: The dict construction unnecessarily wraps iterables in
list() before calling sorted(); change the entries that read "languages":
sorted(list(languages)) and "categories": sorted(list(categories)) to simply use
sorted(languages) and sorted(categories) respectively (update the code where the
dict is built so the sorted() calls accept the iterables directly).
- Around line 98-99: The docstring for function process_entity contains a typo
("Worker for Generates assets"); update it to a correct, concise phrasing such
as "Worker that generates assets for a single Country/Market." to fix grammar
and clarity in the process_entity docstring.
- Around line 177-183: The DEV branch currently hardcodes output_dir =
Path("docs/downloads/ics") and ignores the --output flag; modify the block under
if args.dev to use args.output (e.g., set output_dir = Path(args.output) or
respect args.output when present) before calling clean_output_dir(output_dir),
so args.output is honored in DEV mode; keep the rest of the DEV overrides
(target_years, target_countries, target_financial) unchanged and only replace
the hardcoded Path with the args.output-derived Path.
- Around line 35-39: The clean_output_dir(path) function currently calls
shutil.rmtree on a user-supplied path with no validation; update it to validate
the path before deleting by ensuring the resolved path is a subpath of the
project root (or check for a specific sentinel file inside the directory) and
refuse to operate if the path resolves to a filesystem root or home directory
(e.g., path.resolve() == Path("/") or path.resolve() == Path.home()) or is
outside the project directory; if validation fails, raise a clear exception
instead of deleting, otherwise proceed to rmtree and recreate the directory.
- Around line 147-148: Replace the silent "except Exception: pass" blocks with
proper exception logging: catch the exception as e (e.g., "except Exception as
e") and call the module logger (or logger.exception) to record the error plus
contextual identifiers such as entity, year, language in the first block and
subdivision in the second block so you can trace which item failed; after
logging, continue to the next iteration to preserve existing behavior. Ensure
you reference the existing loop variables (entity, year, language, subdivision)
in the log message and use logger.exception or logger.error with traceback for
full diagnostics.
- Around line 217-221: The as_completed loop currently calls future.result()
directly which will re-raise any exceptions from process_entity and crash before
the manifest is written; wrap the future.result() call in a try/except inside
the loop (catch Exception), log the error and the future identity (or the input
entity), and continue so other futures still contribute to manifest; ensure you
do not lose partial results by leaving manifest population as-is and allowing
the final manifest write to run even if some futures failed (or explicitly write
a partial manifest in the except branch) — update the loop around futures and
the exception handling to reference the futures/as_completed loop and
process_entity results accordingly.
- Around line 113-118: Set default_lang from instance.default_language before
computing languages, then change the languages fallback to use that default
rather than hardcoding ["en"]; specifically, move the default_lang assignment
above where languages is defined and replace the languages fallback expression
(instance.supported_languages if instance.supported_languages else ["en"]) with
(instance.supported_languages if instance.supported_languages else
[default_lang]); also remove any hardcoded "Thailand" or "en_US" example logic
related to supported languages so the fallback uniformly uses default_lang;
leave subdiv_codes and categories logic unchanged.
There was a problem hiding this comment.
Pull request overview
Adds a new site-asset generation script intended to produce downloadable holiday calendars (.ics) plus JSON previews/manifests for supported countries and financial markets.
Changes:
- Introduces
scripts/generate_site_assets.pyto generate per-entity/per-year.icsand.jsonfiles. - Adds parallelized processing via
ProcessPoolExecutorto speed up generation. - Produces an
index.jsonmanifest containing entity metadata (languages, categories, subdivisions).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
KJhellico
left a comment
KJhellico
left a comment
There was a problem hiding this comment.
General suggestion: simpler is better. :)
… with repository standards.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_site_assets.py`:
- Around line 117-118: The loop that calls write_assets is silently skipping
ChildEntity objects when h_obj.subdiv != subdiv; add a diagnostic log inside
that branch to record when a h_obj is skipped including h_obj identifier (e.g.,
h_obj.subdiv or h_obj.code/name), the expected subdiv value, and context (lang,
cat, year_dir) so developers can trace why files weren’t generated; locate the
check around the call to write_assets (the conditional using h_obj.subdiv ==
subdiv) and insert a processLogger or logging.debug/info line that includes
those symbols.
---
Duplicate comments:
In `@scripts/generate_site_assets.py`:
- Around line 67-96: Wrap the entire body of the worker function
(process_entity) in a top-level try/except to prevent uncaught exceptions from
escaping executor.map: catch Exception around all metadata extraction and file
operations (including calls to get_subdivision_aliases(), reading
supported_categories/supported_languages/default_language, and
year_dir.mkdir()), log the error with context (code, name, year) and return a
safe value (e.g., None or a dict indicating failure) so the main executor.map
iteration can continue and the manifest-writing logic still runs; alternatively,
add similar exception handling around the executor.map consumption in main() to
catch worker exceptions, log them, and continue processing.
- Around line 71-73: The fallback language is computed after languages, so
languages uses the hardcoded ["en_US"] instead of the instance.default_language;
compute default_lang first (use getattr(instance, "default_language", "en_US"))
and then set languages = instance.supported_languages if
instance.supported_languages else [default_lang], leaving categories =
instance.supported_categories unchanged and updating any references to
languages/default_lang accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_site_assets.py`:
- Around line 126-153: The current main() uses
ProcessPoolExecutor().map(process_entity, work_items) but if a worker raises an
uncaught exception the map will propagate and stop iteration losing partial
manifest; wrap the for result in executor.map(...) loop in a try/except to catch
any Exception (or BaseException for extreme cases like MemoryError), log the
error, and continue/flush the manifest (i.e., ensure manifest writing still
runs). Specifically update the loop in main() that iterates over executor.map to
handle and recover from exceptions raised during iteration so
manifest[etype][code] entries already collected are preserved and written out.
- Around line 32-48: The function write_assets currently returns early when
h_obj is falsy, skipping creation of .ics/.json files; instead, handle empty
HolidayBase by always generating files: remove the early return and ensure you
still instantiate ICalExporter(h_obj) and call exporter.save_ics(...) for an
empty calendar, and write an empty-but-valid JSON (e.g. "[]") to (year_dir /
f"{filename_base}.json"). Keep the try/except around exporter.save_ics and the
JSON write so errors are caught, and use filename_base and year_dir as in the
existing code paths.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
scripts/generate_site_assets.py (3)
140-144:⚠️ Potential issue | 🟡 Minor
executor.mappropagates worker exceptions and aborts iteration — partial manifest is lost.If any
process_entitycall raises an exception that escapes its internaltry/except(e.g.,MemoryError, pickle failure),executor.mapre-raises it, stopping the loop beforeindex.jsonis written. The previously flagged (optional) fix still applies.🛡️ Defensive wrap
with ProcessPoolExecutor() as executor: - for result in executor.map(process_entity, work_items): - if result: - etype, code, meta = result - manifest[etype][code] = meta + try: + for result in executor.map(process_entity, work_items): + if result: + etype, code, meta = result + manifest[etype][code] = meta + except Exception as e: + print(f"Fatal worker error; writing partial manifest. ({e})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate_site_assets.py` around lines 140 - 144, executor.map will re-raise worker exceptions and abort the loop, losing a partially-built manifest; change the pattern around ProcessPoolExecutor so that you submit tasks with executor.submit(process_entity, item) and iterate over concurrent.futures.as_completed(futures) to collect results into manifest, catching and logging exceptions per-future (so MemoryError/pickle errors don't stop processing) and still write out index.json from the partially-populated manifest; update references to executor.map -> executor.submit/as_completed, keep using process_entity and manifest, and ensure any raised exceptions are logged but do not stop other tasks from being processed.
130-132:⚠️ Potential issue | 🟠 Major
shutil.rmtreeon a hardcoded-but-overridable path still lacks a safety check.If
OUTPUT_DIRis ever misconfigured or the script is adapted to accept an external path, this call nukes the tree with no validation. The past review proposed requiring the path to remain under the project root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate_site_assets.py` around lines 130 - 132, Before calling shutil.rmtree(OUTPUT_DIR) add a safety check that verifies OUTPUT_DIR.resolve() is a subpath of the project root and is not a dangerous absolute path (e.g., "/" or home). Use the project root Path (e.g., PROJECT_ROOT or derive with Path(__file__).resolve().parents[n]) and assert OUTPUT_DIR.resolve().is_relative_to(PROJECT_ROOT.resolve()) (or compare path parts/prefix if Python <3.9), and raise an error or skip removal if the check fails; only then call shutil.rmtree(OUTPUT_DIR) and proceed to OUTPUT_DIR.mkdir(...).
34-35:⚠️ Potential issue | 🟡 MinorEmpty holiday objects still silently skipped.
if not h_obj: returnshort-circuits before writing any file when there are zero holidays for a year/lang/category/subdiv combo. If the portal expects a file to always exist per combination, this will produce broken download links. The past review flagged this; consider writing an empty-but-valid.icsinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate_site_assets.py` around lines 34 - 35, The current early return on "if not h_obj: return" silently skips generating files for combinations with zero holidays; change this so that when h_obj is empty you instead build and write an empty-but-valid .ics (a VCALENDAR with no VEVENTs) using the same writer path used for non-empty h_obj outputs (i.e., the code that writes holiday .ics files), so download links always have a file; keep the same filename/metadata and only omit VEVENT entries when h_obj is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/generate_site_assets.py`:
- Around line 140-144: executor.map will re-raise worker exceptions and abort
the loop, losing a partially-built manifest; change the pattern around
ProcessPoolExecutor so that you submit tasks with
executor.submit(process_entity, item) and iterate over
concurrent.futures.as_completed(futures) to collect results into manifest,
catching and logging exceptions per-future (so MemoryError/pickle errors don't
stop processing) and still write out index.json from the partially-populated
manifest; update references to executor.map -> executor.submit/as_completed,
keep using process_entity and manifest, and ensure any raised exceptions are
logged but do not stop other tasks from being processed.
- Around line 130-132: Before calling shutil.rmtree(OUTPUT_DIR) add a safety
check that verifies OUTPUT_DIR.resolve() is a subpath of the project root and is
not a dangerous absolute path (e.g., "/" or home). Use the project root Path
(e.g., PROJECT_ROOT or derive with Path(__file__).resolve().parents[n]) and
assert OUTPUT_DIR.resolve().is_relative_to(PROJECT_ROOT.resolve()) (or compare
path parts/prefix if Python <3.9), and raise an error or skip removal if the
check fails; only then call shutil.rmtree(OUTPUT_DIR) and proceed to
OUTPUT_DIR.mkdir(...).
- Around line 34-35: The current early return on "if not h_obj: return" silently
skips generating files for combinations with zero holidays; change this so that
when h_obj is empty you instead build and write an empty-but-valid .ics (a
VCALENDAR with no VEVENTs) using the same writer path used for non-empty h_obj
outputs (i.e., the code that writes holiday .ics files), so download links
always have a file; keep the same filename/metadata and only omit VEVENT entries
when h_obj is empty.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3282 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 310 310
Lines 18583 18583
Branches 2378 2378
=========================================
Hits 18583 18583 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'd like to see workflow execution log. |
|
@KJhellico sir , I've made the changes. Since today is the #woc deadline, I'm hoping to get as much of this wrapped up as I can. Whenever you have a spare moment, let me know if there is anything else I can do today! |
Visual issue: in preview, the first year's header is not separated from the first line. |
|
Default selected category should be "Public", not "ALL". |
I agree with setting the default category to 'Public'. but if 'Public' holidays aren't available for a specific country, what should we use as the fallback category? I'm also on board with en_US. My initial thought for defaulting to the regional language was for quicker .ics downloads in the user's native language, but I'm perfectly fine with switching the default to en_US |
That's impossible. |
…reshjoshij/holidays into feat/3168-ics-download-portal
yaah I misunderstood the 'supported category' section in the README. I've updated the defaults to 'Public' and en_US now. |
| from pathlib import Path | ||
|
|
||
| import mkdocs_gen_files | ||
|
|
There was a problem hiding this comment.
I think the following approach would be optimal: we host only the data (JSON and ICS) on GitHub Pages, and then download.js script retrieves it from there. It would also be good to make the script first check for the presence of "local" data (using a relative path, as it does now) and, if available, use it - this will allow the site to be deployed anywhere, including localhost.
There was a problem hiding this comment.
I think the following approach would be optimal: we host only the data (JSON and ICS) on GitHub Pages, and then
download.jsscript retrieves it from there. It would also be good to make the script first check for the presence of "local" data (using a relative path, as it does now) and, if available, use it - this will allow the site to be deployed anywhere, including localhost.
thank you! @KJhellico I think this solves the final puzzle. It's so much better that users can now download directly within the page without being redirected. I've updated the Python script and added the local-first fallback to download.js. Really appreciate the brilliant idea!
|
| if os.environ.get("GITHUB_ACTIONS") == "true": | ||
| OUTPUT_DIR = Path("site/downloads/ics") | ||
| else: | ||
| OUTPUT_DIR = Path("docs/downloads/ics") |
There was a problem hiding this comment.
| if os.environ.get("GITHUB_ACTIONS") == "true": | |
| OUTPUT_DIR = Path("site/downloads/ics") | |
| else: | |
| OUTPUT_DIR = Path("docs/downloads/ics") | |
| OUTPUT_DIR = Path("site/downloads/ics") |
| - name: Install Docs Dependencies | ||
| run: uv sync --frozen --no-default-groups --group build --group docs --link-mode=copy | ||
|
|
||
| - name: Compile Translations | ||
| run: uv run --no-sync scripts/l10n/generate_mo_files.py | ||
|
|
||
| - name: Build Site | ||
| run: make doc | ||
|
|
||
| - name: Generate Holiday Data | ||
| run: uv run --no-sync scripts/generate_site_assets.py | ||
|
|
||
| - name: Upload Pages Artifact |
There was a problem hiding this comment.
| - name: Install Docs Dependencies | |
| run: uv sync --frozen --no-default-groups --group build --group docs --link-mode=copy | |
| - name: Compile Translations | |
| run: uv run --no-sync scripts/l10n/generate_mo_files.py | |
| - name: Build Site | |
| run: make doc | |
| - name: Generate Holiday Data | |
| run: uv run --no-sync scripts/generate_site_assets.py | |
| - name: Upload Pages Artifact | |
| - name: Install dependencies | |
| run: uv sync --frozen --no-default-groups --group build --link-mode=copy | |
| - name: Generate ICalendar data | |
| run: | | |
| uv run --no-sync scripts/l10n/generate_mo_files.py | |
| uv run --no-sync scripts/generate_site_assets.py | |
| - name: Upload GitHub Pages artifact |
But it's better to add a separate target (e.g., icalendar) to the Makefile and run "Generate ICalendar data" stage using make.
Proposed change
This Draft PR introduces the Python script (scripts/generate_site_assets.py) to generate .ics files, & .json previews
Closes #3168
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.