fix(adapters): enforce structured unsupported-operation behavior - #36
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes how adapters signal unsupported feature surfaces by introducing a structured UnsupportedAdapterOperationError and updating lightweight adapters/tests/reporting so capability gaps are classified as “unsupported” rather than indistinguishable silent no-ops.
Changes:
- Add
UnsupportedAdapterOperationErrorplusExcelAdapter.unsupported_operation(...)to raise structured unsupported-operation failures. - Replace silent
passno-ops withself.unsupported_operation(...)in pyexcel/pylightxl/xlwt adapters. - Update failure explanation rendering and tests/docs to recognize and enforce unsupported-feature signaling.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/excelbench/harness/adapters/base.py | Introduces structured unsupported-operation exception + helper raiser and maps unsupported to UNSUPPORTED_FEATURE. |
| src/excelbench/harness/adapters/pyexcel_adapter.py | Replaces unsupported write-surface no-ops with structured unsupported-operation raises. |
| src/excelbench/harness/adapters/pylightxl_adapter.py | Replaces unsupported write-surface no-ops with structured unsupported-operation raises. |
| src/excelbench/harness/adapters/xlwt_adapter.py | Replaces unsupported tier-2 write-surface no-ops with structured unsupported-operation raises. |
| src/excelbench/results/failure_explainer.py | Adds explicit rendering for DiagnosticCategory.UNSUPPORTED_FEATURE. |
| tests/test_adapter_base.py | Adds coverage that unsupported_operation() maps to UNSUPPORTED_FEATURE. |
| tests/test_lightweight_adapters.py | Updates Pyexcel tests to expect raises; adds Pylightxl unsupported-write assertions. |
| tests/test_adapters.py | Adds Xlwt unsupported-operation assertion; adds Calamine close-workbook test. |
| CONTRIBUTING.md | Documents requirement to raise UnsupportedAdapterOperationError via unsupported_operation(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 571700fbcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Motivation
pass) for unsupported read/write surfaces which made capability gaps indistinguishable from semantic regressions.Description
UnsupportedAdapterOperationErrorand a helperExcelAdapter.unsupported_operation(...)for adapters to raise when a feature is not implemented.self.unsupported_operation(...)insrc/excelbench/harness/adapters/pyexcel_adapter.py,pylightxl_adapter.py, andxlwt_adapter.py.src/excelbench/results/failure_explainer.pyto explicitly renderDiagnosticCategory.UNSUPPORTED_FEATUREas an unsupported-capability explanation (not a semantic regression).tests/test_adapter_base.py,tests/test_lightweight_adapters.py, andtests/test_adapters.py, and add a short contributor note inCONTRIBUTING.mdrequiring adapters to raiseUnsupportedAdapterOperationErrorfor unsupported surfaces.Testing
uv run ruff checkagainst the modified files and fixed style issues; the targetedruffrun passed.uv run pytest -o addopts='' tests/test_adapter_base.py tests/test_lightweight_adapters.py tests/test_adapters.py, but collection failed in this environment due to a missing optional dependency (xlwings) required by other import-time modules; the tests and harness-level collection succeed in a full dev environment with the standard extras installed.mypyin this environment and observed existing repo-wide/stub-related failures (unrelated to these changes) due to missing stubs/dependencies; these are pre-existing baseline issues and were not introduced by this patch.Codex Task