Repository navigation
Accept broken symlinks as path arguments of dandi validate - #1937
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1937 +/- ##
==========================================
+ Coverage 78.32% 78.35% +0.03%
==========================================
Files 92 92
Lines 14055 14109 +54
==========================================
+ Hits 11008 11055 +47
- Misses 3047 3054 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`dandi validate` already handles annexed files whose content has not been
fetched (the broken symlinks of a DataLad dataset) according to
`--missing-file-content`: `error`, `skip`, or `only-non-data`. Those policies
were applied only when such a file was reached through its directory,
though: given directly on the command line, the file never got past argument
parsing, because `click.Path(exists=True)` follows the link and rejected it
with "does not exist". Check the path with `lexists()` instead, so that
dandi validate --missing-file-content=skip sub-01/sub-01.nwb
works the same as validating `sub-01/`, while paths that truly do not exist
are still rejected with the same error.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA
4f316f7 to
addfb66
Compare
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Subclassing click.Path and checking with os.path.lexists() first is the right approach. click.Path.convert always follows links through os.stat() and has no option to use lstat. A callback= would be harder to reuse, and dropping exists=True would turn a clean usage error for a mistyped path into an unclear failure later on. The test covers the cases that matter.
A few things to tighten before merging (details inline):
- Stop
exists=Trueandresolve_path=Truefrom being passed back in, since either one undoes the fix. - Handle
allow_dash, and build the error message the same way click does (format_filename,_()). - The deprecated
validate-bidsstill usesclick.Path(exists=True). Either switch it or say that leaving it is intentional.
Generated by Claude Code
Address review on the `click.Path` subclass accepting broken symlinks: - Rename `ExistingPath` to `PathOrBrokenSymlink`, which says what sets it apart, and move it to `dandi/cli/base.py` with the other reusable `ParamType`s. - Force `exists=False` and refuse `exists=True` or `resolve_path=True`: the former would bring back click's own check, which rejects broken links, and the latter would replace an annexed link with its `.git/annex/objects/...` target. - Match `click.Path.convert` more closely: skip the check for `-` when `allow_dash` applies, and build the message with gettext and `click.utils.format_filename()` as click does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
|
Leaving as is. Generated by Claude Code |
Which I will say, Claude also offered to fix separately from all this validation effort, if you are interested? Might increase coverage % |
yarikoptic
left a comment
There was a problem hiding this comment.
I hate to do it, but I started to wonder if we could choose a better name.
Also I think docstring should not talk in such gory detail about why -- move into commit msg
| return "[" + ",".join(self.values) + ",all]" | ||
|
|
||
|
|
||
| class PathOrBrokenSymlink(click.Path): |
There was a problem hiding this comment.
stroke me only now that the name is a bit misleading/could be improved given that it is forever to be part of our public interface ...
I kinda see the rationale for it but it kinda begs a question "What about a not broken symlink?". Overall any symlink is also a Path strictly speaking, so it is really the absense of lexists option in addition to exists (also at pathlib level somewhat lacking such parallel to os.path. interfaces) hindering the core situation here...
so I wonder what is a potentially better name here?
overall it could even be just an overloaded Path but accepting the lexists flag in addition to exists (exists=False, lexists=True even makes sense if someone is making command like 'fix-broken-links'). Type checking etc would help to ensure using our overloaded one (dandi.cli.Path) where we want to allow providing broken links as well. WDYT?
There was a problem hiding this comment.
Agreed, done in 56bb1a5: dandi.cli.base.Path, a click.Path with a lexists flag next to exists; validate now uses Path(lexists=True, dir_okay=True). exists=True keeps click's meaning (and wins if both are given); lexists=True only refuses resolve_path=True, which would swap a link for its target. Tests cover lexists, exists, and both. I kept it in dandi/cli/base.py next to EnumChoice/ChoiceList rather than re-exporting from dandi.cli; happy to add that if you prefer.
Generated by Claude Code
There was a problem hiding this comment.
Hmm sorry about that - Codybot should have proposed some naming alternative before assuming
I'd propose either UnresolvedPath or LinkAwarePath: your choice or propose your own
There was a problem hiding this comment.
@yarikoptic (going with working name of LinkAwarePath for now since it assumes the least about the target)
Per review: rather than a special-purpose type whose name raises "what about a symlink that is not broken?", provide `dandi.cli.base.Path`, a `click.Path` taking a `lexists` flag next to `exists`. With `lexists=True` a path must satisfy `os.path.lexists()`, so it may be a symlink whose target is missing, which `exists=True` (following the link) rejects. `dandi validate` needs that so its `--missing-file-content` policies (error, skip, only-non-data) also apply to an annexed file whose content was not fetched when that file is given directly on the command line, not only when reached via its directory. `lexists=True` refuses `resolve_path=True`, which would replace such a link with its `.git/annex/objects/...` target; `exists=True` keeps its click meaning. The docstring now describes only the behavior, leaving the rationale to this message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Add project-level .claude/settings.json denying the GitHub MCP tools that post, edit, or resolve comments and reviews on pull requests and issues. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Path clashed with pathlib.Path; the docstring now explains why a symlink-aware path type is needed rather than how it works. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
…nit__ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Keep that configuration personal instead, as in con/fscacher#113. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic
left a comment
There was a problem hiding this comment.
Great, thanks! Let's just squash for merge
|
🚀 PR was released in |
…hamilton-ast647 The base was rebased onto master (with #1937 and #1939 squash-merged) and now uses fscacher >= 0.5.0's memoize_path(custom_fingerprint=...). Resolve accordingly: - Drop memoize_source and its tests: get_metadata, get_neurodata_types, nwb_has_external_links and _validate now use memoize_path(custom_fingerprint=readable_fingerprint). - Drop ExistingPath in favor of LinkAwarePath(lexists=True) from #1937. - Keep the streaming support (_validate reading from a Readable) and its test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr


Another pre-PR splintered out of #1933 since it does actually affect the CLI today (granted, IDK why someone WOULD use the CLI in that way today on a datalad dataset, but they COULD)
dandi validatealready handles annexed files whose content has not been fetched (the broken symlinks of a DataLad dataset) according to--missing-file-content:error,skip, oronly-non-data. Those policies were applied only when such a file was reached through its directory, though: given directly on the command line, the file never got past argument parsing, becauseclick.Path(exists=True)follows the link and rejected it with "does not exist". Check the path withlexists()instead, so thatworks the same as validating
sub-01/, while paths that truly do not exist are still rejected with the same error.Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA