Skip to content

Address review feedback and improve examples - #5

Merged
Spiffical merged 1 commit into
masterfrom
codex/address-review-feedback
Jul 15, 2026
Merged

Address review feedback and improve examples#5
Spiffical merged 1 commit into
masterfrom
codex/address-review-feedback

Conversation

@Spiffical

Copy link
Copy Markdown
Owner

What changed

  • correct audio download summary semantics by separating newly downloaded files from files already available locally
  • make documentation-figure downloads atomic and validate ONC response media types
  • support the declared Pillow 9.0 minimum and make temporary-file cleanup reliable
  • harden calendar and legend layout handling with regression coverage
  • make documentation examples self-contained and align the README with the audio-first beginner workflow
  • ignore generated MkDocs site output

Why

Copilot review comments across the recent documentation and performance PRs identified several genuine correctness, compatibility, and copy/paste usability issues. The broader audit also found a misleading long-range sampling example and an outdated README entry path.

Impact

Download summaries now accurately distinguish work performed from files already present, documentation generation fails with clear errors for invalid ONC responses, and first-time users can run the documented workflows without guessing setup variables.

Validation

  • pytest -q: 51 passed, 2 integration tests deselected
  • mkdocs build --strict: passed
  • 31 Python README/documentation blocks compiled successfully
  • python -m compileall -q onc_hydrophone_data scripts tests: passed
  • python -m pip check: passed
  • staged whitespace and credential scans: passed

@Spiffical
Spiffical marked this pull request as ready for review July 15, 2026 03:35
Copilot AI review requested due to automatic review settings July 15, 2026 03:35
@Spiffical
Spiffical merged commit f0bb548 into master Jul 15, 2026
1 check passed

Copilot AI left a comment

Copy link
Copy Markdown

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 addresses prior review feedback by improving correctness of audio download summaries, hardening documentation figure generation (atomic writes + content-type validation), and updating docs/examples to be self-contained and aligned with the “audio-first” workflow.

Changes:

  • Refines audio download summary semantics by separating newly downloaded files from locally available/skipped files.
  • Makes docs-figure audio download atomic and validates ONC response status/content type; improves Pillow compatibility and temp-file cleanup.
  • Updates README/MkDocs examples to include shared setup blocks and fixes long-range sampling examples; ignores generated MkDocs site/ output.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_plot_layouts.py Adds tolerance to legend placement assertion and adds regression coverage for missing last-bin start in calendar plotting.
tests/test_download_efficiency.py Updates expectations for new download summary semantics and adds coverage for newly-downloaded vs available counts.
tests/test_docs_figure_generation.py Adds focused tests for atomic ONC audio preview download and content-type rejection.
scripts/generate_docs_figures.py Improves Pillow resampling compatibility, ensures temp cleanup, and makes ONC preview download atomic with validation.
README.md Updates “Quick Start” to the audio-first workflow and aligns CLI examples/device/date paths.
pyproject.toml Uses structured TOML list for pytest addopts (aligned with pytest>=8).
onc_hydrophone_data/utils/plotting.py Hardens calendar plotting when last bin start is missing (returns None cleanly).
onc_hydrophone_data/data/downloader/onc_downloads.py Adds files_available and corrects downloaded-vs-skipped semantics in audio download summary.
docs/troubleshooting.md Clarifies reuse of the same downloader instance and fixes missing import in an example.
docs/onc_spectrogram_options.md Makes examples self-contained with shared setup and clearer starter range guidance.
docs/downloads.md Adds shared setup block and adjusts sampling/audio examples to be copy/paste runnable.
docs/custom_spectrograms.md Makes the example self-contained by including downloader/config setup.
docs/audio_downloads.md Adds shared setup variables and fixes sampling example variables for usability.
.gitignore Ignores generated MkDocs site/ output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants