Skip to content

fix: add incremental build support#92

Merged
tylerdave merged 3 commits intomainfrom
incremental-builds
Apr 5, 2026
Merged

fix: add incremental build support#92
tylerdave merged 3 commits intomainfrom
incremental-builds

Conversation

@tylerdave
Copy link
Copy Markdown
Member

  • adds incremental builds
  • overwrite by default
  • clear contents w/ flag

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 5, 2026

Docs Site preview: https://45bcea96.rockgarden.pages.dev

claude[bot]

This comment was marked as outdated.

claude[bot]

This comment was marked as outdated.

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Good incremental build implementation overall — the manifest design is clean and the happy-path (rename with same page count) works correctly. Two issues to flag: one new bug and one previously raised issue still present.


if incremental and manifest:
current_slugs = {p.slug for p in pages}
for old_slug, entry in list(manifest.pages.items()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: stale output not cleaned up when a page is deleted. When needs_full_rebuild returns True (e.g. page count drops because a page was deleted), the old manifest is discarded and replaced with a fresh empty one. During the build loop all current pages are written into manifest.pages, so by the time stale cleanup runs at line 881, manifest.pages only contains current slugs — old_slug not in current_slugs is never true and the deleted page's HTML file is never removed. test_removed_page_triggers_full_rebuild only asserts skipped_count == 0, not that the orphaned file is gone. Fix: save the loaded old manifest before replacing it, then use its pages dict as the source for stale cleanup regardless of whether a full or incremental rebuild ran.

Comment thread src/rockgarden/cli.py
shutil.rmtree(output_dir)
else:
raise typer.Exit(0)
if clean and _output_dir_has_contents(output_dir):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Previously flagged and still present: interactive confirmation prompt removed. The prior review noted that the typer.confirm("Output directory … exists. Delete contents?") guard was dropped entirely, so a plain rockgarden build into an existing output dir now silently overwrites in place. The docs now document this behaviour, but losing the prompt is still a regression — users who accidentally target the wrong directory get no warning before their previous output is partially overwritten. Consider restoring the prompt for the non---incremental, non---clean path, or at minimum emitting a one-line notice when writing into a non-empty output dir.

@tylerdave tylerdave merged commit 17cfd13 into main Apr 5, 2026
8 checks passed
@tylerdave tylerdave deleted the incremental-builds branch April 5, 2026 18:17
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.

1 participant