Set DataLad-next 1.0.0b2 dependency, adjust accordingly #23
Conversation
This introduces a versioned dependency on DataLad-next (replacing github main). DataLad dependency is also increased by one patch version - not essential for this extension's code, but 0.18.2. includes a fix for compatibility with most recent git-annex versions.
This commit introduces validate_defaults and tailor_for_dataset in parameter validation, removing the need for require_dataset and resolve_path inside the __call__ function.
Tests of parameterization checks will now test for ConstraintError instead of ValueError. This does not matter for the outcome of the test (ConstraintError is a subclass of ValueError), but now that we have a dedicated error we might as well be specific.
An unused import is removed, and test code is blackened.
| from datalad.interface.common_opts import ( | ||
| nosave_opt, | ||
| save_message_opt, |
There was a problem hiding this comment.
No related to this PR, but I want to challenge the need for this feature.
There was a problem hiding this comment.
The need for importing the parameters from common_opts? Or the need for having --message and --nosave?
My idea for --nosave was that currently the form (and also report) export can write one or multiple forms into one file. --nosave was supposed to allow repeating the command to write several forms into several files, and capturing them in a single, manual, save afterwards. But we could later add a many-forms-to-separate-files variant of the export command, and make the problem go away. Or we could do away with --nosave right now (should --message stay?) - is that what you suggest?
There was a problem hiding this comment.
Without deeply thinking about it again, I'd say this command should either
- always save
- never save
If it always saves, it should support setting a message (see datalad/datalad#3316)
However, this creates complications. See datalad/datalad#3896 for an entrypoint to some. Few more keywords are: recursive saving of superdatasets, etc.
Just saving also does not necessarily provide an indication of the origin of the save information.
Without wanting to propse a full concept: It may just be better to never save for this command.
- if people want to export multiple, they run multiple command, and save ones at the end -- no need for more code
- if people want to do things recursively, the combine this command with
for_each_dataset, and safe at then end or individually as they desired - if people want to have provenance capture of where that information is coming from, they combine any of the above with
datalad run
There was a problem hiding this comment.
Interesting - I'll probably take it to a separate issue to think of it a bit more. FTR, the current behavior when saving is to use the provided message or craft one based on the form names. And the command would refuse operation if a file is in a subdataset, if that matters.
There was a problem hiding this comment.
It matters in that an operation in a hierarchical dataset tree already requires more saving than the command can provide.
| @@ -142,13 +138,15 @@ class ExportProjectXML(ValidatedInterface): | |||
| dict( | |||
| url=EnsureURL(required=["scheme", "netloc", "path"]), | |||
| outfile=EnsurePath(), | |||
| dataset=EnsureDataset(installed=True, purpose="export redcap report"), | |||
| dataset=EnsureDataset(installed=True, purpose="export REDCap project XML"), | |||
| credential=EnsureStr(), | |||
| metadata_only=EnsureBool(), | |||
| survey_fields=EnsureBool(), | |||
| message=EnsureStr(), | |||
| save=EnsureBool(), | |||
There was a problem hiding this comment.
Same remark as above. I believe this is a non-feature.
datalad_redcap/export_project_xml.py
Outdated
| @@ -217,8 +207,8 @@ def __call__( | |||
|
|
|||
| # unlock the file if needed, and write contents | |||
| if unlock: | |||
| ds.unlock(res_outfile) | |||
| with open(res_outfile, "wt") as f: | |||
| ds.unlock(outfile) | |||
| @@ -163,20 +153,20 @@ def __call__( | |||
|
|
|||
| # unlock the file if needed, and write contents | |||
| if unlock: | |||
| ds.unlock(res_outfile) | |||
| @@ -92,11 +88,13 @@ class ExportReport(ValidatedInterface): | |||
| url=EnsureURL(required=["scheme", "netloc", "path"]), | |||
| report=EnsureStr(), | |||
| outfile=EnsurePath(), | |||
| dataset=EnsureDataset(installed=True, purpose="export redcap report"), | |||
| dataset=EnsureDataset(installed=True, purpose="export REDCap report"), | |||
| credential=EnsureStr(), | |||
| message=EnsureStr(), | |||
| save=EnsureBool(), | |||
That's the point of inviting reviews :) Thanks for all the comments.
Agree - I had already written that idea in #12, but don't have an elegant solution yet. |
Without it, flow control is not handed to the caller, and results are rendered unconditionally, even when a caller disables it. Co-authored-by: Michael Hanke <[email protected]>
Extends the change from e8e9cda Good practice: without yield, flow control is not handed to the caller, and results are rendered unconditionally, even when a caller disables it.
Updates format of intersphinx_mapping to deal with deprecation of the old format in Sphinx 6.2 Not sure if we need the mapping at all, but it's a small change. https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html#confval-intersphinx_mapping
This PR sets a "proper" versioned dependency for
datalad-next >= 1.0.0b2(replacing previous dependency on GitHub main) in anticipation of the redcap extension release. Closes #19The PR includes adjustments of parameter validation to benefit from the new features added between beta1 and beta2 pre-releases of -next (
validate_defaults,tailor_for_dataset).