refactor: centralize unusable file path checks - #468
Conversation
|
👋 Hi @Kaileshwar16 — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
The issue named four sites. There are six: render_bundle and render_deploy_bundle each run the same pair on config.requirements_file. Both carried the comment 'for the reason above', and 'above' was the explanation this PR moved onto the helper, so leaving them behind left two comments pointing at nothing.
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM. Merging.
Two notes.
You are right that both runtime sites have coverage now, so ignore point 3 of the issue. I checked all of them by deleting each call individually, and every one turns a test red.
The issue also undercounted. There are six sites, not four: render_bundle and render_deploy_bundle each run the same pair on config.requirements_file. Both of those carried # FileNotFoundError, not IsADirectoryError, for the reason above., and "above" was the explanation this PR moves onto the helper, so leaving them behind left two comments pointing at nothing. I pushed a commit routing them through the helper too, dropping the now-unused errno and os imports, and adding the three-argument reasoning to the helper comment.
Validated on the merged result: ruff, ruff format, ty, 1713 passed / 6 skipped.
helper body neutered 12 failed
helper drops the directory clause 6 failed
helper drops the third argument 4 failed
site: LocalStorageRoot.fetch 3 failed
site: fetch_uri local branch 3 failed
site: render_bundle pipeline 2 failed
site: render_bundle requirements 1 failed
site: render_deploy_bundle pipeline 2 failed
site: render_deploy_bundle reqs 1 failed
The good first issue pool is best kept for newcomers, so for your next one, #480 is yours if you want it, or the higher-leverage version: run HFlow against a real corpus (Egocentric-10K or Egocentric-100K on Hugging Face) and report what breaks, what is slow, and what is awkward. That is the feedback we cannot generate ourselves.
Summary
Centralizes the repeated unusable-file-path validation used across storage and runtime code.
The new private
_refuse_unusable_file_path()helper lives instorage.py, since that module already owns local path handling and both runtime modules already depend on it.Changes
Added
_refuse_unusable_file_path(path: Path)insrc/hflow/storage.pyReplaced the duplicated directory / missing-file checks in:
LocalStorageRoot.fetch()fetch_uri()render_deploy_bundle()render_bundle()Preserved the existing behavior:
FileNotFoundErrorwitherrno.EISDIRFileNotFoundErrorwitherrno.ENOENTfilenameand OS error text remain unchangedKept the reasoning for using
FileNotFoundErrorinstead ofIsADirectoryErrorin one place on the helperValidation
Ran:
Result:
Also confirmed the four existing storage regression tests pass with the helper in place.
As requested in the issue, I temporarily replaced the helper body with
passand confirmed all four regression tests fail becauseFileNotFoundErroris no longer raised.I also checked the current test suite for the four call sites. On the current
main, both runtime sites now have missing-file and directory-path coverage, so the issue note that at least one site is untested appears to be outdated.Closes #467