connector(filesystem): Refactor resource_for_reader args into data class

[amotl]

About

It looks like all arguments to resource_for_reader could be bundled by using a data class with named attributes, so it also becomes easier to add new ones across the code base, which currently needs different touch points. What do you think about this, @coderabbitai and @hampsterx?

References

• Based on: Filesystem: Add reader for Excel XLSX files #123 (comment)
• Supersedes: 📝 CodeRabbit Chat: Refactor resource_for_reader arguments into a data class #146

[amotl]

@coderabbitai: Please review, also taking the conversation in #146 into account.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Current PR metadata (`#169`) =="
gh pr view 169 --json number,title,body,author,baseRefName,headRefName,files,url

echo
echo "== Files changed in `#169` =="
gh pr diff 169 --name-only

echo
echo "== Earlier PR metadata (`#146`) =="
gh pr view 146 --json number,title,body,author,state,url

echo
echo "== Review comments and discussion from `#146` =="
gh api repos/panodata/omniload/issues/146/comments --paginate --jq '.[] | {user: .user.login, created_at, body}'
echo
gh api repos/panodata/omniload/pulls/146/comments --paginate --jq '.[] | {user: .user.login, path, line, body}'

echo
echo "== Outline of likely changed Python files in current checkout =="
for f in $(gh pr diff 169 --name-only | rg '\.py$'); do
  echo "-- $f --"
  ast-grep outline "$f" --items all || true
done

echo
echo "== Unified diff for `#169` =="
gh pr diff 169

Length of output: 47336

---

@amotl: Thanks — I’ve taken the discussion from #146 into account, especially the earlier naming/placement thread around moving the reader reference object into omniload/source/filesystem/model.py. I’ll review #169 with that context in mind rather than re-litigating the superseded approach.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.96%. Comparing base (8ae0bb0) to head (17da06c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
omniload/source/filesystem/adapter.py	71.42%	2 Missing ⚠️
omniload/source/filesystem/impl/remote.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   51.89%   51.96%   +0.06%     
==========================================
  Files         196      196              
  Lines        9139     9153      +14     
==========================================
+ Hits         4743     4756      +13     
- Misses       4396     4397       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c34c18a8-df73-498b-aca6-8bbfab99b47f

📥 Commits

Reviewing files that changed from the base of the PR and between 6e3e06b and 17da06c.

📒 Files selected for processing (5)

• omniload/source/filesystem/adapter.py
• omniload/source/filesystem/impl/local.py
• omniload/source/filesystem/impl/remote.py
• omniload/source/filesystem/model.py
• tests/main/filesystem/test_local.py

🚧 Files skipped from review as they are similar to previous changes (5)

• omniload/source/filesystem/model.py
• tests/main/filesystem/test_local.py
• omniload/source/filesystem/impl/remote.py
• omniload/source/filesystem/adapter.py
• omniload/source/filesystem/impl/local.py

---

Summary by CodeRabbit

• New Features

  • Standardized filesystem-based loading around a single reference object, making local and remote sources behave consistently.

• Bug Fixes

  • Improved handling of CSV headerless reads by deriving column names from available column settings.
  • Updated filesystem source setup for local, S3, GCS, and SFTP paths to use the same input format.

• Tests

  • Adjusted filesystem test coverage to match the updated source-loading flow.

Walkthrough

This PR adds a FilesystemReference dataclass and updates filesystem reader construction to pass that object through resource_for_reader. Local, GCS, S3, and SFTP source implementations, plus the local test stub, are adjusted to use the new call shape.

Changes

FilesystemReference-based resource_for_reader refactor

Layer / File(s)	Summary
FilesystemReference data contract omniload/source/filesystem/model.py	Adds FilesystemReference with filesystem, bucket, glob, reader, optional page, and column type fields.
resource_for_reader signature and logic update omniload/source/filesystem/adapter.py	Changes resource_for_reader to accept FilesystemReference and build readers from ref fields.
Source wiring and test stub alignment omniload/source/filesystem/impl/local.py, omniload/source/filesystem/impl/remote.py, tests/main/filesystem/test_local.py	Updates filesystem sources to construct FilesystemReference and updates the test stub to match the new signature.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

• panodata/omniload#160: Also changes the resource_for_reader path in omniload/source/filesystem/adapter.py and related reader/resource wiring.

Suggested reviewers: hampsterx

Poem

I hopped through refs both neat and bright,
A tiny bundle now holds the flight.
Buckets, globs, and readers too,
All passed as one—how tidy-new!
🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: refactoring resource_for_reader arguments into a data class.
Description check	✅ Passed	The description is clearly related to the change and explains the intent of bundling resource_for_reader arguments.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch resource-reader-dataclass

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/impl/local.py`:
- Line 11: The top-level import of FilesystemReference in local.py breaks the
lazy startup pattern by importing omniload.source.filesystem.model too early
because it pulls in fsspec at module load. Move the FilesystemReference import
into dlt_source alongside the other filesystem imports, and keep the local
module free of that top-level dependency so it only loads when the function
runs.

🪄 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: 0af111e1-fd06-490f-ac97-df0f5fc2ffe3

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae0bb0 and 6e3e06b.

📒 Files selected for processing (5)

• omniload/source/filesystem/adapter.py
• omniload/source/filesystem/impl/local.py
• omniload/source/filesystem/impl/remote.py
• omniload/source/filesystem/model.py
• tests/main/filesystem/test_local.py