Skip to content

refactor(commands): extract cross-cutting commands from files.py#102

Closed
ypriverol wants to merge 4 commits into
refactor/files-py-providersfrom
refactor/files-py-commands
Closed

refactor(commands): extract cross-cutting commands from files.py#102
ypriverol wants to merge 4 commits into
refactor/files-py-providersfrom
refactor/files-py-commands

Conversation

@ypriverol
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #101. Extracts the three cross-cutting commands (download_files_by_url, download_files_by_list, download_px_raw_files and their helpers) from pridepy/files/files.py into a new pridepy/commands/ package. No behaviour change.

files.py size: 1254 → 995 LOC (21% reduction). The spec target of <700 LOC was overly optimistic; the remaining 995 is largely backward-compat shims for the ~40 methods that tests patch via Files.X. Migrating tests to patch the new modules directly would let the shims be removed, but that's deferred to a separate PR.

What moved

Old location New location
Files.download_px_raw_files + _normalize_px_xml_url + _parse_px_xml_for_raw_file_urls commands/proteomexchange.py (94 LOC)
Files.download_files_by_list commands/by_list.py (58 LOC)
Files.download_files_by_url + 6 URL helpers (_extract_pride_accession, _validate_urls_checksums, _http_download_url, _ftp_download_url, _dispatch_url_scheme, _download_single_url) commands/by_url.py (254 LOC)

Backward compatibility

Files keeps shim staticmethods for every moved function. Tests use patch.object(Files, "_http_download_url") and patch.object(Files, "_download_single_url") — both still intercept because the new commands/by_url.py internal helpers route through Files.X (lazy import) for the patch-sensitive ones.

Tests

68 passed, 4 skipped — same as PR #101 baseline. No assertion changes.

Verification

  • Live smoke test: pridepy download-file-by-name -a MSV000080175 -f params.xml produces MD5 43d87368d705c3f380c1d030b14850c4 (matches the pre-refactor baseline).
  • All 8 test_download_resilience.py tests pass.
  • All 4 test_download_by_url.py tests pass (the ones patching Files._http_download_url, _ftp_download_url, _download_single_url).
  • All 9 test_download_by_list.py tests pass.

Base-branch note

This PR targets refactor/files-py-providers (PR #101) so the diff is small. Once #101 merges to dev, update this PR's base.

Test plan

  • CI passes
  • wc -l pridepy/files/files.py reports < 1000
  • Smoke test: pridepy download-file-by-name -a MSV000080175 -f params.xml produces MD5 43d87368d705c3f380c1d030b14850c4

ypriverol added 4 commits May 27, 2026 18:03
Empty scaffold for the follow-up refactor that extracts cross-cutting
commands (download_files_by_url, download_files_by_list,
download_px_raw_files) from Files into their own modules. No code moved
yet. No behaviour change. Test suite green at 68 passed, 4 skipped.
…roteomexchange.py

Moved download_px_raw_files, _normalize_px_xml_url,
_parse_px_xml_for_raw_file_urls from Files into
commands/proteomexchange.py. Files keeps shim re-exports.
Also removed now-unused xml.etree.ElementTree import from files.py.

No behaviour change. Test suite green.
Moved download_files_by_list from Files into commands/by_list.py.
Files keeps a shim re-export.

No behaviour change. Test suite green.
Moved download_files_by_url and its 6 helpers (_extract_pride_accession,
_validate_urls_checksums, _http_download_url, _ftp_download_url,
_dispatch_url_scheme, _download_single_url) from Files into
commands/by_url.py. Files keeps shim re-exports for each.

Internal calls to patch-sensitive helpers (_http_download_url,
_ftp_download_url, _dispatch_url_scheme, _download_single_url) go
through Files.X (lazy import) so existing test patches like
patch.object(Files, '_http_download_url') keep intercepting.

files.py drops below 1000 LOC.

No behaviour change. Test suite green at 68 passed, 4 skipped.
@qodo-code-review
Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2bc20dc7-ff3b-4143-9e47-bcb48af71e6d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/files-py-commands

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@ypriverol
Copy link
Copy Markdown
Contributor Author

Superseded by #103, which consolidates PR #100, #101, and #102 into a single branch (feat/files-py-modular-architecture) targeting dev. All 4 commits from this PR are preserved verbatim in #103. Please review and merge there instead.

@ypriverol ypriverol closed this May 27, 2026
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.

1 participant