Bring wallpapers up to KKL conventions - #49
Conversation
Extract repeated patterns from task scripts: - Path constants (WALLPAPERS_OUTPUT_DIR, CONFIG_FILE, STATE_DIR) - detect_screen_resolution() — replaces 5 copies of system_profiler pipeline - resolution_width/height() — replaces cut -d'x' pattern - require_command() — replaces ad-hoc command -v checks - require_config() — replaces ad-hoc config file checks
- Source common.sh for shared paths and helpers - Replace inline system_profiler with detect_screen_resolution() - Replace ad-hoc command/config checks with require_command/require_config - Replace BASH_SOURCE gymnastics with $MISE_CONFIG_ROOT - Add #USAGE headers to cli task - Update ai task reference from CLAUDE.md to README.md
14 tests covering path constants, resolution helpers, require_command, require_config, and test fixtures.
- Delete CLAUDE.md — tool codebases should be agent-agnostic - Add Development section to README with architecture, resolution presets, and test instructions - Add README.tsx JSX template for dynamic README generation
Tutorial scripts, error messages, and hints now reference the wp alias consistently. Internal task dispatch and Hammerspoon config (non-shiv context) retain mise run.
baby-joel
left a comment
There was a problem hiding this comment.
Clean extraction — the lib + BATS pattern is solid and the BASH_SOURCE → $MISE_CONFIG_ROOT swap is the right call. One bug that needs fixing before merge:
Bug: stale $OUTPUT_DIR in generate task
.mise/tasks/generate:106 — ls -la "$OUTPUT_DIR" still references the old variable. OUTPUT_DIR was replaced by source lib/common.sh (which exports WALLPAPERS_OUTPUT_DIR), so this line now expands to ls -la "", which silently lists the current directory instead of the wallpapers dir.
# line 106
ls -la "$OUTPUT_DIR" # ← should be $WALLPAPERS_OUTPUT_DIRNit: tutorial tasks still hardcode paths
tutorial/apply, tutorial/generate, and tutorial/navigate still define OUTPUT_DIR and SCRIPT_DIR locally instead of sourcing lib/common.sh. I see the PR body says this is deferred — that's fine, but worth tracking so it doesn't drift. Maybe a # TODO: source lib/common.sh comment in each, or a follow-up issue.
Question: README.tsx import resolution
README.tsx imports from readme/src/components — is this resolved via a local readme package or a KKL tool? Not blocking, just want to understand how README.tsx is intended to be executed.
Everything else looks good: the BATS test coverage is thorough, require_command/require_config deduplicate nicely, the cli USAGE headers are a good addition, and the mise run → wp string updates are consistent across tasks.
Replace stale $OUTPUT_DIR reference with $WALLPAPERS_OUTPUT_DIR from lib/common.sh. Add TODO comments to tutorial tasks for future migration to shared lib. Addresses review feedback from baby-joel on #49.
|
Fixed in eb3cb2a:
|
baby-joel
left a comment
There was a problem hiding this comment.
Both items addressed — $OUTPUT_DIR → $WALLPAPERS_OUTPUT_DIR fix looks correct, and the TODO comments in the tutorial tasks are clear breadcrumbs. LGTM, good to merge.
2522075 to
3a849ba
Compare
|
Continued the revival branch with a final convention pass in 3a849ba. What changed on top of Ikma's work:
Verified locally:
CI is green: https://github.com/KnickKnackLabs/wallpapers/actions/runs/25585936138 |
Summary
Brings the wallpapers codebase in line with KnickKnackLabs tooling conventions (per
creating-a-codebase.md).Changes
Shared library (
lib/common.sh)WALLPAPERS_OUTPUT_DIR,WALLPAPERS_CONFIG_FILE,WALLPAPERS_STATE_DIRdetect_screen_resolution()— replaces 5 duplicatesystem_profilerpipelinesresolution_width()/resolution_height()— parse WxH stringsrequire_command()/require_config()— consistent dependency and config checksTask refactoring
lib/common.shinstead of defining their own constantsBASH_SOURCEgymnastics replaced with$MISE_CONFIG_ROOT#USAGEheaders added toclitaskaitask updated to reference README.md instead of deleted CLAUDE.mdTesting
lib/common.shtesttask addedtest_helper.bashwith shared fixturesConfiguration
bats = "1.13.0"added tomise.tomltask_output = "interleave"setting added (passescodebase lint:mise-settings)Documentation
CLAUDE.md— tool codebases should be consumable by both agents and humans; the README is the single source of truthREADME.tsxfor dynamic README generationNot changed
Sources/,Package.swift,Tests/)mise runreferences in tutorial scripts (cosmetic cleanup deferred)