fix(client): give specific error for blank/whitespace pathspec components#3261
Conversation
When a pathspec like "Flow/ /Step" was passed (whitespace-only component), the existing code would either silently pass the split or produce a confusing wrong-component-count error, rather than identifying the real problem (blank/whitespace component). Add an explicit check that reports the exact position(s) of blank components before the length validation runs, giving users a clear, actionable error message. Fixes Netflix#948 (partial)
Greptile SummaryThis PR adds an explicit check for blank or whitespace-only components in a pathspec string before the existing length validation, raising
Confidence Score: 4/5Safe to merge; the change is a small guard that fires only when a pathspec contains a blank component, leaving all valid pathspecs completely unchanged. The fix is logically correct and well-placed, with one minor UX note: positions in the error message are 0-indexed, which may be slightly confusing for users who think of path components as starting at 1. No files require special attention beyond the single changed guard in metaflow/client/core.py. Important Files Changed
Reviews (1): Last reviewed commit: "fix(client): improve error message for b..." | Re-trigger Greptile |
| raise MetaflowInvalidPathspec( | ||
| "Pathspec '%s' has an empty or blank component at position(s) %s. " | ||
| "Each '/' must separate non-empty path parts." | ||
| % (pathspec, ", ".join(str(p) for p in blank)) | ||
| ) |
There was a problem hiding this comment.
The positions are reported as 0-based indices, but end-users mentally count path components from 1 (e.g.,
"Flow/ /Step" — component 2 is blank, not component 1). Using 1-based positions in the error message would be more intuitive here.
| raise MetaflowInvalidPathspec( | |
| "Pathspec '%s' has an empty or blank component at position(s) %s. " | |
| "Each '/' must separate non-empty path parts." | |
| % (pathspec, ", ".join(str(p) for p in blank)) | |
| ) | |
| raise MetaflowInvalidPathspec( | |
| "Pathspec '%s' has an empty or blank component at position(s) %s. " | |
| "Each '/' must separate non-empty path parts." | |
| % (pathspec, ", ".join(str(p + 1) for p in blank)) | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3261 +/- ##
=========================================
Coverage ? 29.08%
=========================================
Files ? 381
Lines ? 52516
Branches ? 9267
=========================================
Hits ? 15275
Misses ? 36204
Partials ? 1037 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
When a pathspec contains a whitespace-only component (e.g.
"Flow/ /Step"), the existing code produces a confusing error:str.split("/")silently)In both cases the user has no clear signal about what is wrong.
Fix
Add an explicit check before the component-count validation that:
MetaflowInvalidPathspecwith the offending position(s) called outNo behaviour change for valid pathspecs.
Fixes #948 (partial — error message quality)