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

Return non-ok result records with a generic result renderer #357

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

adswa
Copy link
Member

@adswa adswa commented Mar 1, 2023

Even though the comment says that logging has handled any non-ok, non-meta-extract action, I believe that relying on logging solely is not ok and bad UX. Logging does not ensure that users get to see the log messages. A failure can go entirely unnoticed as users would see nothing returned in their terminal when their configured log level does not match the log level the errors are logged on.

Even though the comment says that logging has handled any non-ok, non-meta-extract
action, I believe that relying on logging solely is not ok and bad UX.
Logging does not ensure that users get to see the log messages. A failure can
go entirely unnoticed as users would see nothing returned in their terminal when
their configured log level does not match the log level the errors are logged on.
@adswa
Copy link
Member Author

adswa commented Mar 1, 2023

Actually, now that I actually looked into how it would be done in next, I think metalad should just go for next parameter validation. This diff would achieve the same result:

diff --git a/datalad_metalad/extract.py b/datalad_metalad/extract.py
index 4fd7b45..0faaa37 100644
--- a/datalad_metalad/extract.py
+++ b/datalad_metalad/extract.py
@@ -30,12 +30,16 @@ from typing import (
 from uuid import UUID
 
 from dataclasses import dataclass
-
+from datalad_next.commands import (
+    EnsureCommandParameterization,
+    ValidatedInterface,
+)
 from datalad.distribution.dataset import Dataset
 from datalad.distribution.dataset import (
     datasetmethod,
     EnsureDataset,
 )
+from datalad_next.constraints import EnsurePath
 from datalad.interface.base import (
     Interface,
     build_doc,
@@ -91,7 +95,7 @@ class ExtractionArguments:
 
 
 @build_doc
-class Extract(Interface):
+class Extract(ValidatedInterface):
     """Run a metadata extractor on a dataset or file.
 
     This command distinguishes between dataset-level extraction and
@@ -131,6 +135,11 @@ class Extract(Interface):
     on the whether file-level- or dataset-level extraction is requested.
     """
 
+    _validator_ = EnsureCommandParameterization(dict(
+        path=EnsurePath(lexists=True),
+    ))
+
     result_renderer = "tailored"
 

Copy link
Collaborator

@christian-monch christian-monch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks a lot.

@christian-monch
Copy link
Collaborator

Actually, now that I actually looked into how it would be done in next, I think metalad should just go for next parameter validation. This diff would achieve the same result:

Great work, thanks! We should definitely use the new validators throughout metalad. I did not look at them closely yet because they have not made it into core yet, and if they are still in next, one more extension would be required to run metalad.

Thanks for issue #359

@christian-monch christian-monch changed the base branch from master to maint_0.4 March 8, 2023 15:54
@christian-monch
Copy link
Collaborator

Thank you @adswa . I changed the destination branch to maint_0.4 and will merge it.

@christian-monch christian-monch merged commit e66fc01 into datalad:maint_0.4 Mar 8, 2023
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.

2 participants