Conversation
Documentation build overview
31 files changed ·
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 52.84% 53.88% +1.03%
==========================================
Files 202 202
Lines 9323 9463 +140
==========================================
+ Hits 4927 5099 +172
+ Misses 4396 4364 -32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amotl
force-pushed
the
excel
branch
4 times, most recently
from
51ba162 to
5c7c8ae
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
Summary by CodeRabbit
WalkthroughThis PR adds XLSX support to the filesystem connector, wires the new reader through adapter resolution, registers ChangesXLSX filesystem support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AzureSource
participant Adapter
participant ReadExcel
participant Polars
AzureSource->>Adapter: FilesystemReference(hints=blob_hints(...))
Adapter->>Adapter: readers(...).with_resources("read_excel")
Adapter->>ReadExcel: bind sheet_name from ref.hints
ReadExcel->>Polars: read_excel(file bytes, sheet_name)
Polars-->>ReadExcel: parsed rows
ReadExcel-->>Adapter: yield named records
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
Binary files shouldn't be added to this repository. Let's start a dedicated one to collect data samples for testing purposes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@omniload/source/filesystem/format/readers.py`:
- Around line 88-107: The Excel sheet hint is being read under the wrong hint
key, so user-provided `#sheet=...` never reaches `read_excel`. Update the reader
wiring in `resource_for_reader()` to pass `sheet_name=ref.hints.get("sheet")`
(or normalize the hint key before dispatch), keeping `read_excel()` unchanged
except for consuming the correct value.
In `@omniload/source/filesystem/impl/remote.py`:
- Around line 215-216: The UnsupportedEndpointError handling in remote.py should
suppress exception chaining when converting it to the ValueError from
supported_file_format_message("Azure"). Update the except
UnsupportedEndpointError block so the re-raise mirrors the SFTP path and uses an
explicit from None on the ValueError, keeping the behavior consistent and
satisfying Ruff’s B904 check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49f53f91-c293-4912-a9d6-97daffd58c46
⛔ Files ignored due to path filters (1)
tests/assets/create_replace.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (9)
docs/changelog.mddocs/supported-sources/file.mdomniload/source/filesystem/adapter.pyomniload/source/filesystem/format/readers.pyomniload/source/filesystem/format/registry.pyomniload/source/filesystem/impl/remote.pypyproject.tomltests/main/filesystem/test_remote.pytests/warehouse/filesystem/test_excel.py
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (5)
- omniload/source/filesystem/format/registry.py
- pyproject.toml
- tests/warehouse/filesystem/test_excel.py
- tests/main/filesystem/test_remote.py
- omniload/source/filesystem/adapter.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@omniload/source/filesystem/format/readers.py`:
- Around line 88-107: The Excel sheet hint is being read under the wrong hint
key, so user-provided `#sheet=...` never reaches `read_excel`. Update the reader
wiring in `resource_for_reader()` to pass `sheet_name=ref.hints.get("sheet")`
(or normalize the hint key before dispatch), keeping `read_excel()` unchanged
except for consuming the correct value.
In `@omniload/source/filesystem/impl/remote.py`:
- Around line 215-216: The UnsupportedEndpointError handling in remote.py should
suppress exception chaining when converting it to the ValueError from
supported_file_format_message("Azure"). Update the except
UnsupportedEndpointError block so the re-raise mirrors the SFTP path and uses an
explicit from None on the ValueError, keeping the behavior consistent and
satisfying Ruff’s B904 check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49f53f91-c293-4912-a9d6-97daffd58c46
⛔ Files ignored due to path filters (1)
tests/assets/create_replace.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (9)
docs/changelog.mddocs/supported-sources/file.mdomniload/source/filesystem/adapter.pyomniload/source/filesystem/format/readers.pyomniload/source/filesystem/format/registry.pyomniload/source/filesystem/impl/remote.pypyproject.tomltests/main/filesystem/test_remote.pytests/warehouse/filesystem/test_excel.py
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (5)
- omniload/source/filesystem/format/registry.py
- pyproject.toml
- tests/warehouse/filesystem/test_excel.py
- tests/main/filesystem/test_remote.py
- omniload/source/filesystem/adapter.py
🛑 Comments failed to post (2)
omniload/source/filesystem/format/readers.py (1)
88-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the hint key used by the parser/model vs. the adapter binding. rg -nP '\bsheet(_name)?\b' \ omniload/source/filesystem/adapter.py \ omniload/source/filesystem/router.py \ omniload/source/filesystem/model.pyRepository: panodata/omniload
Length of output: 1088
🌐 Web query:
polars read_excel accept bytes input and sheet_name None default sheet💡 Result:
Polars' polars.read_excel function supports bytes input directly [1][2]. While earlier versions experienced a regression that briefly restricted this [1][3], current versions accept raw bytes as the source parameter [1][2]. Regarding sheet selection, the function does not use None to default to the first sheet. Instead: 1. Default Behavior: If neither sheet_name nor sheet_id is specified, Polars defaults to loading sheet number 1 (the first sheet) [4][5]. 2. Loading All Sheets: To load all sheets at once, you should set sheet_id=0 [4][5]. This returns a dictionary mapping sheet names to DataFrames [4][5]. Passing bytes directly as the source is standard, but if you encounter issues in older environments, you can wrap the bytes in a io.BytesIO object, which is treated as a file-like object by the library [6][2].
Citations:
- 1: pola-rs/polars#20526
- 2: https://github.com/pola-rs/polars/blob/main/py-polars/src/polars/io/spreadsheet/functions.py
- 3: pola-rs/polars#20630
- 4: https://docs.pola.rs/py-polars/html/reference/api/polars.read_excel.html
- 5: https://docs.pola.rs/api/python/dev/reference/api/polars.read_excel.html
- 6: https://docs.pola.rs/api/python/stable/reference/api/polars.read_excel.html
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== adapter ==\n' sed -n '130,155p' omniload/source/filesystem/adapter.py printf '\n== router ==\n' sed -n '80,110p' omniload/source/filesystem/router.py printf '\n== model ==\n' sed -n '30,50p' omniload/source/filesystem/model.py printf '\n== read_excel ==\n' sed -n '80,115p' omniload/source/filesystem/format/readers.pyRepository: panodata/omniload
Length of output: 4787
Use
sheetwhen binding Excel hints.
#sheet=...is parsed intoref.hints["sheet"], butresource_for_reader()looks upref.hints.get("sheet_name"), so user-supplied sheet selection never reachesread_excel. Normalize the key at the boundary or bindsheet_name=ref.hints.get("sheet").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@omniload/source/filesystem/format/readers.py` around lines 88 - 107, The Excel sheet hint is being read under the wrong hint key, so user-provided `#sheet=...` never reaches `read_excel`. Update the reader wiring in `resource_for_reader()` to pass `sheet_name=ref.hints.get("sheet")` (or normalize the hint key before dispatch), keeping `read_excel()` unchanged except for consuming the correct value.omniload/source/filesystem/impl/remote.py (1)
215-216: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Chain the re-raise with
from None(B904).Ruff flags this re-raise inside the
exceptclause. The SFTP path already usesfrom Nonefor the same mapping; mirror it here to silence the expectedUnsupportedEndpointErrorchain and keep the lint gate green.🔧 Proposed fix
except UnsupportedEndpointError: - raise ValueError(supported_file_format_message("Azure")) + raise ValueError(supported_file_format_message("Azure")) from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.except UnsupportedEndpointError: raise ValueError(supported_file_format_message("Azure")) from None🧰 Tools
🪛 Ruff (0.15.20)
[warning] 216-216: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@omniload/source/filesystem/impl/remote.py` around lines 215 - 216, The UnsupportedEndpointError handling in remote.py should suppress exception chaining when converting it to the ValueError from supported_file_format_message("Azure"). Update the except UnsupportedEndpointError block so the re-raise mirrors the SFTP path and uses an explicit from None on the ValueError, keeping the behavior consistent and satisfying Ruff’s B904 check.Source: Linters/SAST tools
1 participant
About
A connector for reading spreadsheets from Excel XLSX files was dearly missing.
References