Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR includes XML definition files in the packaged library by updating the packaging configuration, addressing an issue where these files were missing after pip installation. Additionally, it incorporates copier template updates that bump various dependency versions and improve CI configuration.
- Adds vendor XML files to package data in pyproject.toml
- Updates dependency versions for development tools
- Incorporates upstream template changes for CI/CD improvements
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds XML vendor files to package data and bumps version to 0.2.6 |
| CHANGELOG.md | Documents the fix for including vendor files in the packaged library |
| tests/unit/snapshot.py | Fixes property name for file extension in test class |
| ruff.toml | Updates linting configuration and comment wording |
| ruff-test.toml | Adds unfixable rule for pytest context managers |
| .pre-commit-config.yaml | Updates tool versions and adds new security checks |
| .github/workflows/pre-commit.yaml | Adds persist-credentials configuration |
| .github/workflows/get-values.yaml | Adds GitHub context debugging and security configuration |
| .devcontainer/install-ci-tooling.py | Updates tool versions and improves curl command with retry logic |
| .devcontainer/devcontainer.json | Updates devcontainer hash |
| .copier-answers.yml | Updates template commit reference |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| WriteMode.TEXT | ||
| ) # for some reason the default is binary, but it should be text to make diffs easier to read | ||
| _file_extension = "xml" | ||
| file_extension = "xml" |
There was a problem hiding this comment.
Missing underscore prefix. The property should be _file_extension to match the naming convention of other private properties in this class like _write_mode.
| file_extension = "xml" | |
| _file_extension = "xml" |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR updates tooling and configuration: bumps package version to 0.2.6 and adds package data inclusion; upgrades dev tooling versions; adjusts GitHub Actions steps and checkout credential persistence; refreshes pre-commit hooks and adds workflow validation; tweaks Ruff configs; updates changelog; and renames a test attribute. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant R as Runner
participant Repo as Repository
GH->>R: Trigger get-values workflow
R->>R: Display full GitHub context (jq)
R->>Repo: actions/checkout@vX with persist-credentials: false
Note over R: Remaining steps proceed unchanged
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.copier-answers.yml(1 hunks).devcontainer/devcontainer.json(1 hunks).devcontainer/install-ci-tooling.py(2 hunks).github/workflows/get-values.yaml(1 hunks).github/workflows/pre-commit.yaml(1 hunks).pre-commit-config.yaml(5 hunks)CHANGELOG.md(1 hunks)pyproject.toml(3 hunks)ruff-test.toml(1 hunks)ruff.toml(3 hunks)tests/unit/snapshot.py(1 hunks)
🔇 Additional comments (15)
.copier-answers.yml (1)
2-2: LGTM! Template version update.The copier template version bump is an administrative update with no functional impact on the project configuration.
CHANGELOG.md (1)
17-21: LGTM! Changelog entry properly documents the packaging fix.The changelog entry correctly documents the inclusion of vendor files in the packaged library, aligns with the PR objective, and follows Keep a Changelog formatting conventions.
.github/workflows/pre-commit.yaml (2)
38-38: LGTM! Security improvement.Disabling credential persistence (
persist-credentials: false) prevents Git credentials from being exposed in the workspace after checkout, following security best practices.
43-44: LGTM! Security improvement.Consistent application of
persist-credentials: falseacross both checkout scenarios strengthens the security posture of the workflow..devcontainer/devcontainer.json (1)
64-64: LGTM! Administrative hash update.The devcontainer context hash is automatically managed by a pre-commit hook. This update has no functional impact on the development environment.
pyproject.toml (3)
3-3: LGTM! Version bump aligns with changelog.The version increment to 0.2.6 correctly reflects the patch-level fix documented in the changelog.
47-47: LGTM! Package data inclusion correctly configured.The package-data specification
{"pyalab" = ["vendor_files/**/*.xml"]}will correctly include all XML files from the vendor_files directory and its subdirectories in the packaged wheel, addressing the PR's main objective.
31-31: Verify syrupy 5.0.0 upgrade
Confirm no breaking changes affect the snapshot extension—inspect the syrupy 5.0.0 changelog for renamed attributes (e.g.file_extension) and run the full test suite (especiallytests/unit/snapshot.py) to ensure all snapshot tests pass without deprecation warnings..github/workflows/get-values.yaml (2)
27-31: LGTM! Useful debugging addition.The new step to display the full GitHub context provides valuable debugging information for troubleshooting workflow issues without affecting workflow functionality.
35-36: LGTM! Security improvement.Setting
persist-credentials: falseprevents Git credentials from persisting after checkout, consistent with the security improvements made in the pre-commit workflow.ruff.toml (3)
48-48: LGTM! Consistent with other docstring rules.Adding D105 (docstrings for magic methods) to the ignore list is consistent with the existing policy of not requiring docstrings for various code elements (D100-D107).
72-75: LGTM! Formatting improvement.The reformatted unfixable list improves readability while maintaining the same set of rules.
90-90: LGTM! Comment clarification.The wording change from "little enough" to "rarely enough" improves clarity while maintaining the same meaning.
.devcontainer/install-ci-tooling.py (2)
68-68: LGTM! Excellent resilience and security improvements.The enhanced curl command adds multiple layers of resilience and security:
- Connection and operation timeouts prevent indefinite hangs
- Retry logic with backoff handles transient failures
--proto '=https'restricts to HTTPS only, preventing protocol downgrade attacksThe timeout values are well-balanced:
--max-time 40accounts for the total operation including retries, while--connect-timeout 20handles per-connection attempts.
10-11: Approve version bumps: UV 0.8.22 and pnpm 10.17.1 confirmed available; no security advisories affect these versions.
| WriteMode.TEXT | ||
| ) # for some reason the default is binary, but it should be text to make diffs easier to read | ||
| _file_extension = "xml" | ||
| file_extension = "xml" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify attribute name change aligns with syrupy 5.0.0 API.
This change from _file_extension to file_extension (removing the underscore prefix) contradicts a previous review comment that suggested it should be private to match the naming convention of _write_mode. This might be required by the syrupy 5.0.0 upgrade. Please verify that this change aligns with the syrupy 5.0.0 API requirements for SingleFileSnapshotExtension.
Search the web for syrupy 5.0.0 API documentation:
Additionally, check the syrupy source code:
🏁 Script executed:
#!/bin/bash
# Description: Check if file_extension needs to be public in syrupy 5.x
# Search for how file_extension is used in test files
echo "=== Checking file_extension usage ==="
rg -n --type=py 'file_extension' tests/
# Look for similar snapshot extension patterns in the codebase
echo "=== Checking for other snapshot extensions ==="
ast-grep --pattern $'class $_ (SingleFileSnapshotExtension):
$$$
'Length of output: 494
🌐 Web query:
Does syrupy 5.0.0 SingleFileSnapshotExtension require file_extension to be a public attribute instead of _file_extension?
💡 Result:
Short answer: No — SingleFileSnapshotExtension expects the (protected) _file_extension attribute (as used in the docs and examples). See the issue showing subclasses overriding _file_extension (and the subsequent fix for multi-part extensions). [1][2]
Sources:
- README / built-in extensions listing (SingleFileSnapshotExtension). [1]
- GitHub issue demonstrating subclassing by setting _file_extension and related fix. [2]
Override protected _file_extension instead of defining public file_extension
SingleFileSnapshotExtension subclasses must override the protected _file_extension attribute, not introduce a public file_extension.
Location: tests/unit/snapshot.py:14
🤖 Prompt for AI Agents
In tests/unit/snapshot.py around line 14, the subclass defines a public
attribute file_extension = "xml" but the framework expects subclasses to
override the protected attribute _file_extension; change/remove the public
file_extension and set the protected attribute _file_extension = "xml" instead
so SingleFileSnapshotExtension uses the intended protected override.
There was a problem hiding this comment.
this is the correct syntax for the new v5 of syrupy
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
syrupy 5.0.0 breaking changes file_extension attribute SingleFileSnapshotExtension
💡 Result:
Short answer: there is no Syrupy 5.0.0 breaking change that removes the file‑extension attribute from SingleFileSnapshotExtension. The SingleFile extensions still expect/allow an internal _file_extension override (and historically used that private attribute), and support for multi‑part extensions (e.g. "png.zip") was added in an earlier fix. See: [CHANGELOG / v4.9.x], issue about multi‑part extensions (fixed/merged), and the SingleFile behavior discussion. [1][2][3]
Sources:
- CHANGELOG (serializer/amber notes / v4.9.x showing serializer changes discussed for v5 milestone). [1]
- Issue: "having a dot in the file extension causes unexpected behavior" (discusses _file_extension and shows fix for multi‑part extensions was merged). [2]
- Issue: "Crash with SingleFileSnapshotExtension" (explains SingleFile uses a byte/string serializer and the private _file_extension usage/behavior). [3]
🌐 Web query:
syrupy 5.0.0 release notes changelog migration guide v4 to v5
💡 Result:
Summary of Syrupy 5.0.0 — key release / migration notes (v4 → v5)
- Where to read full notes and docs: Syrupy repo README / v5 pre-release docs. [1]
- Package release (PyPI 5.0.0 published). [2]
What changed (high‑level)
- Major release (breaking changes). Requires Python > 3.9 and pytest >= 8 (v4 required Python >3.8.1 and pytest >=7,<9). [1][2]
- Main branch/docsite moved to the v5 pre-release (readme/doc differences vs. v4 stable docs). [1]
- Matchers: v5 improves matcher composition (compose_matchers) and changes matcher behavior/ APIs (issues and migration discussions exist in the v5 milestone). Example: easier composition for path_type/path_value and new behaviors for float handling. See issue threads for examples. [1][3]
- General: many v5 changes were tracked as milestone issues (ordering of parameterized snapshots, matcher improvements, etc.). Review milestone/issue list for specific behavioral changes affecting snapshots. [3]
Migration checklist (v4 → v5)
- Upgrade runtime/test tooling:
- Move to Python > 3.9 and pytest >= 8. [1][2]
- Update syrupy in your environment:
- pip install --upgrade syrupy (or pin ==5.0.0). [2]
- Review snapshot matcher usage:
- If you use custom matchers or path_type/path_value, check tests for compatibility; consider using compose_matchers where applicable. [1][3]
- Remove or adjust conflicting snapshot libraries:
- If you previously used snapshottest, uninstall it and remove old snapshots (syrupy has guidance for migration from snapshottest). [1]
- Run test suite and update snapshots:
- Run pytest; run pytest --snapshot-update to update snapshots where intended. Inspect any reordered or reformatted snapshots and adjust tests if necessary. [1]
- Check issues/milestone for edge cases:
- Look through the v5 milestone and related issues for any behaviors that match your usage (parameterized ordering, float serialization, xdist limitations, etc.). [3]
If you want, I can:
- fetch and paste the repository CHANGELOG or RELEASE notes (if present) and extract all breaking changes and code examples; or
- scan your repo/tests for likely breaking matcher usages and suggest concrete changes to migrate.
Sources
- Syrupy repository README / v5 notes (main branch). [1]
- PyPI page for syrupy (release 5.0.0). [2]
- Selected v5 milestone/issue discussion (matchers, float handling, ordering). [3]
🏁 Script executed:
#!/bin/bash
# Check the installed syrupy version and look for any examples or documentation
# in the repository about SingleFileSnapshotExtension usage
echo "=== Checking pyproject.toml for syrupy version ==="
rg -n "syrupy" pyproject.toml
echo -e "\n=== Looking for any syrupy documentation or examples in the repo ==="
fd -e md -e txt -e rst | xargs rg -l "syrupy" 2>/dev/null || echo "No documentation files found"
echo -e "\n=== Checking all Python files for SingleFileSnapshotExtension usage ==="
rg -n "SingleFileSnapshotExtension" --type=py
echo -e "\n=== Checking imports from syrupy ==="
rg -n "from syrupy" --type=py
Length of output: 779
@ejfine, thank you for the clarification! I appreciate you confirming that file_extension (public attribute) is the correct syntax for syrupy v5.0.0. I'll note this API change for future reference.
✏️ Learnings added
Learnt from: ejfine
PR: LabAutomationAndScreening/pyalab#52
File: tests/unit/snapshot.py:14-14
Timestamp: 2025-10-07T13:46:20.557Z
Learning: In syrupy 5.0.0, SingleFileSnapshotExtension subclasses should use the public attribute `file_extension` instead of the protected attribute `_file_extension` that was used in syrupy 4.x.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Why is this change necessary?
The XML definition files weren't being included in the packaged library and so weren't available after a
pip installHow does this change address the issue?
Adds them as specified in pyproject.toml
What side effects does this change have?
the wheel is larger
How is this change tested?
Local build and ensuring they're included
Other
Pulled in some copier template updates to bump versions
Summary by CodeRabbit
Bug Fixes
Chores
Documentation