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

Use datalad-nexts constraints system #401

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use datalad-nexts constraints system #401

wants to merge 8 commits into from

Conversation

mih
Copy link
Member

@mih mih commented Oct 21, 2022

Coming with datalad/datalad-next#116

Shrinks gooey code base by 5%.

Tests cannot pass yet. Waiting for datalad-next 0.7.

@mih mih force-pushed the next-constraints branch from 916d29f to 4e949e2 Compare February 17, 2023 09:03
@adswa adswa linked an issue Apr 20, 2023 that may be closed by this pull request
@adswa
Copy link
Member

adswa commented Apr 20, 2023

I have applied a few minimal fixes to this PR such that the CI now passes. I'm not sure, however, if it is in a state you would like it to be in @mih given that there were quite a few developments in datalad-next's constraint system in the meantime.

@mslw
Copy link
Contributor

mslw commented Jun 14, 2023

Revisiting this PR with DataLad 0.18.5 and DataLad-next 1.0.0b3, I can confirm that tests pass locally for me. I also did some basic clicking around.

With this PR branch checked out, right-click to get file content #406 is indeed fixed (as stated previously).

However, new weirdness seems to appear. All inputs become text widgets, and there is an odd default for a list of paths. This is when doing hen doing right-click (on a not installed subdataset) -> Dataset commands -> Get content

... for this PR:

Screenshot from 2023-06-14 18-46-15

... compared to current main

Screenshot from 2023-06-14 18-48-04

I was not able to dig deeper.

@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1550775) 60.44% compared to head (230b8d8) 57.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
- Coverage   60.44%   57.05%   -3.39%     
==========================================
  Files          49       48       -1     
  Lines        3504     3244     -260     
==========================================
- Hits         2118     1851     -267     
- Misses       1386     1393       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adswa
Copy link
Member

adswa commented Jan 2, 2024

I can reproduce the issue @mslw reported. I believe that the reason for the failure to select the correct Input Widget stems from the fact that the isinstance checks of the constraints do not expect ConstraintWithPassthroughs (they check, e.g., if isinstance(constraint, EnsureDataset) ...

(Pdb) p constraint
ConstraintWithPassthrough(AnyOf(EnsureIterableOf(item_constraint=EnsurePath(), min_len=None, max_len=None), EnsurePath()), passthrough=None)
(Pdb) p name
'path'

Here is a patch with a fix:

diff --git a/datalad_gooey/constraints.py b/datalad_gooey/constraints.py
index 71cc889..46c85e9 100644
--- a/datalad_gooey/constraints.py
+++ b/datalad_gooey/constraints.py
@@ -22,6 +22,7 @@ from datalad_next.constraints import (
     NoConstraint,
 )
 from datalad_next.constraints.parameter_legacy import EnsureParameterConstraint
+from datalad_next.constraints.compound import ConstraintWithPassthrough
 from datalad.distribution.dataset import EnsureDataset as CoreEnsureDataset
 from datalad.distribution.dataset import (
     Dataset,
diff --git a/datalad_gooey/param_form_utils.py b/datalad_gooey/param_form_utils.py
index edfccb8..d4fcbc6 100644
--- a/datalad_gooey/param_form_utils.py
+++ b/datalad_gooey/param_form_utils.py
@@ -32,6 +32,7 @@ from .api_utils import (
 from .utils import _NoValue
 from .constraints import (
     AltConstraints,
+    ConstraintWithPassthrough,
     EnsureBool,
     EnsureExistingDirectory,
     EnsureDatasetSiblingName,
@@ -212,6 +213,12 @@ def _get_parameter(
 
     # if we have no idea, use a simple line edit
     type_widget = pw.StrParameter
+
+    if isinstance(constraint, ConstraintWithPassthrough):
+        # to make proper constraint identification below work, get the
+        # internal constraint:
+        constraint = constraint.constraint
+
     ### now some parameters where we can derive semantics from their name
     if isinstance(constraint, EnsureDataset) \
             or isinstance(constraint, EnsureExistingDirectory):

adswa added 3 commits January 2, 2024 10:19
DataLad-next constraints were (in all cases?) ConstraintWithPassthroughs
that slipped by the widget-to-constraint detection. This change extracts
the internal constraint if its within a ConstrainWithPassthrough to make
it functional again.
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.

getting file from right-click context menu violates constraints
4 participants