Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement EnsurePath.for_dataset() and resolve against that dataset #271

Merged
merged 4 commits into from
Mar 3, 2023

Conversation

mih
Copy link
Member

@mih mih commented Mar 2, 2023

This implements DataLad's standard path resolution approach (relative
is relative to CWD, unless a Dataset instance is given).

This now makes it possible to perform this standard resolution pattern
during command parameter validation, like so:

# example validator for a command with two args
EnsureCommandParameterization(
    param_constraints=dict(
        dataset=EnsureDataset(),
        path=EnsurePath(),
    ),
    tailor_for_dataset=dict(path='dataset'),
)

This will:

  • cause the 'dataset' validator to run first
  • auto-generate an EnsurePath variant that resolves against that
    dataset
  • ensures that the command always receives absolute paths, resolved
    against the dataset whenever the rules require it.

Closes #270

The PR also contains an update of the download command to make use of these new abilities. As a result a substantial amount of custom code could be removed that partially duplicated path resolving and argument inspection (between upfront and runtime aspects).

@adswa
Copy link
Member

adswa commented Mar 3, 2023

I have used this check in datalad/datalad-metalad#362 and it was very useful to have!

mih added 2 commits March 3, 2023 08:36
Previously this method only got a `Dataset`-type argument. However,
for path resolution (for example), this information is insufficient,
because DataLad needs to know whether the type of the original argument.

For this reason `DatasetParameter` already existed, and is now also used
for this purpose.

This also simplified the type-annotation -- no need for custom typevars.

It also made sense to move the `DatasetParameter` class to `base.py`,
because every single constraint implementation has to support it (or
use the default implementation from `base.py`.
This implements DataLad's standard path resolution approach (relative
is relative to CWD, unless a `Dataset` instance is given).

This now makes it possible to perform this standard resolution pattern
during command parameter validation, like so:

```py
\# example validator for a command with two args
EnsureCommandParameterization(
    param_constraints=dict(
        dataset=EnsureDataset(),
        path=EnsurePath(),
    ),
    tailor_for_dataset=dict(path='dataset'),
)
```

This will:

- cause the 'dataset' validator to run first
- auto-generate an `EnsurePath` variant that resolves against that
  dataset
- ensures that the command always receives absolute paths, resolved
  against the dataset whenever the rules require it.

Closes datalad#270
mih added 2 commits March 3, 2023 09:22
Now also all aspects of `path` resolution, and deriving a filename
from an input URL are done outside the command implementation and
in respective constraints.

Improtantly, this now also uses a new feature of
`EnsureGeneratorFromFileLike` that can now simply relay per-item
exceptions to a caller (the `download()` command in this case).
This allows for performing comprehensive item constraint evaluation,
have that use regular exceptions (as if done upfront, but here actually
during the runtime of a command, due to the long-running generator
providing command input AKA batch mode). The command gets the exception
yielded (not raised) and can decide what to do with it. `download()`
maintains its previous behavior and reports an 'impossible' result.
The command can now immediately error when it is clear from the input
arguments that no destination filename can be derived (when none is
provided).

When this is not possible, the command continue to yield an 'impossible'
result.

RF tests on invalid command calls into a dedicated function. No need for
an http server in this case.
@mih
Copy link
Member Author

mih commented Mar 3, 2023

Failures are unrelated.

@mih mih merged commit 0e459f9 into datalad:main Mar 3, 2023
@mih mih deleted the resolvepath branch March 3, 2023 10:28
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.

Implement resolve_path as a constraint
2 participants