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

BF: Don't suppress datalad subdatasets output #6035

Merged
merged 2 commits into from Oct 6, 2021

Conversation

DisasterMo
Copy link
Contributor

Changes

Add custom_result_renderer() to Subdatasets command class as a wrapper for default_result_renderer().
Now datalad subdatasets will actually do what it's supposed to: list all present subdatasets without suppressing the output.

Related Issues

Fixes #4770

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #6035 (4259e16) into maint (14398d7) will decrease coverage by 2.00%.
The diff coverage is 100.00%.

❗ Current head 4259e16 differs from pull request most recent head 64d6cbd. Consider uploading reports for the commit 64d6cbd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6035      +/-   ##
==========================================
- Coverage   90.17%   88.17%   -2.01%     
==========================================
  Files         312      312              
  Lines       42134    42139       +5     
==========================================
- Hits        37995    37155     -840     
- Misses       4139     4984     +845     
Impacted Files Coverage Δ
datalad/distribution/get.py 97.90% <ø> (ø)
datalad/local/subdatasets.py 94.21% <100.00%> (-2.34%) ⬇️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/add_readme.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/check_dates.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/export_archive.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/export_to_figshare.py 0.00% <0.00%> (-100.00%) ⬇️
... and 77 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14398d7...64d6cbd. Read the comment docs.

@mih
Copy link
Member

mih commented Oct 5, 2021

Thanks much for this PR!

I would say this PR can go against the maint branch, given its nature of being a fix.

I have presently no idea re the cause of this failure:

https://app.travis-ci.com/github/datalad/datalad/jobs/541349553

======================================================================
2363FAIL: datalad.metadata.tests.test_search.test_within_ds_file_search
2364----------------------------------------------------------------------
2365Traceback (most recent call last):
2366  File "/tmp/dl-miniconda-kkjyqyv4/lib/python3.9/site-packages/nose/case.py", line 198, in runTest
2367    self.test(*self.arg)
2368  File "/tmp/dl-miniconda-kkjyqyv4/lib/python3.9/site-packages/datalad/tests/utils.py", line 742, in _wrap_with_tempfile
2369    return t(*(arg + (filename,)), **kw)
2370  File "/tmp/dl-miniconda-kkjyqyv4/lib/python3.9/site-packages/datalad/metadata/tests/test_search.py", line 241, in test_within_ds_file_search
2371    assert_equal(
2372AssertionError: Lists differ: ['subdataset(impossible): /tmp/datalad_temp[184 chars].id'] != ['audio.music-artist', 'datalad_core.id']
2373
2374First differing element 0:
2375'subdataset(impossible): /tmp/datalad_temp[142 chars]set]'
2376'audio.music-artist'
2377
2378First list contains 1 additional elements.
2379First extra element 2:
2380'datalad_core.id'
2381
2382+ ['audio.music-artist', 'datalad_core.id']
2383- ['subdataset(impossible): '
2384-  '/tmp/datalad_temp_test_within_ds_file_search8g4h9zzx/.datalad/metadata/objects/da/ds-753090a017914e0862098e811d7237 '
2385-  '[path not contained in any matching subdataset]',
2386-  'audio.music-artist',
2387-  'datalad_core.id']

@DisasterMo DisasterMo changed the base branch from master to maint October 5, 2021 12:14
@mih mih added the semver-patch Increment the patch version when merged label Oct 5, 2021
@mih
Copy link
Member

mih commented Oct 5, 2021

@DisasterMo You will need to (cherry-)pick the commit with the actual change, and base that off of maint. Right now the PR is trying to merge master plus that commit into maint -- which we cannot do. If you have difficulties achieving the desired state, please let me know -- I can help out. Thx!

@DisasterMo
Copy link
Contributor Author

Oh, now I understand. I won't need help with that, but why merge it into maint instead of master in the first place?

@mih
Copy link
Member

mih commented Oct 5, 2021

The reason is that we will release from maint rather soon (days or weeks), but not from master for ~6 months. Purely organizational setup. Only fixes can go to maint, but they will reach an audience faster than through master

@DisasterMo DisasterMo force-pushed the bf-4770-subdatasets-no-suppress branch from 8702574 to 0c038cf Compare October 5, 2021 15:22
@mih
Copy link
Member

mih commented Oct 6, 2021

FTR: The test failure posted above is real. I can replicate it locally.

@mih
Copy link
Member

mih commented Oct 6, 2021

I found the cause for the test failure. I cannot push directly to this branch, hence I am attaching the patch.
0001-Disable-result-rendering-for-internal-subdatasets-ca.patch.txt

From 1d5081d06aea747c6f02748d0c87eb2ff773c026 Mon Sep 17 00:00:00 2001
From: Michael Hanke <michael.hanke@gmail.com>
Date: Wed, 6 Oct 2021 08:03:18 +0200
Subject: [PATCH] Disable result rendering for internal `subdatasets()` call in
 `get()`

The yielded results are inspected by `get` and evaluated in the context
of a get operation. A premature rendering by `subdatasets()` itself
is undesireable.
---
 datalad/distribution/get.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/datalad/distribution/get.py b/datalad/distribution/get.py
index b550595d9..161ee258e 100644
--- a/datalad/distribution/get.py
+++ b/datalad/distribution/get.py
@@ -859,7 +859,8 @@ class Get(Interface):
                 recursive=True if path else recursive,
                 recursion_limit=None if path else recursion_limit,
                 return_type='generator',
-                on_failure='ignore'):
+                on_failure='ignore',
+                result_renderer='disabled'):
             if sdsres.get('type', None) != 'dataset':
                 # if it is not about a 'dataset' it is likely content in
                 # the root dataset
-- 
2.33.0

DisasterMo and others added 2 commits October 6, 2021 08:54
The yielded results are inspected by `get` and evaluated in the context
of a get operation. A premature rendering by `subdatasets()` itself
is undesireable.
@DisasterMo DisasterMo force-pushed the bf-4770-subdatasets-no-suppress branch from 0c038cf to 64d6cbd Compare October 6, 2021 06:56
@DisasterMo
Copy link
Contributor Author

@mih Thanks! Should I squash the commits, or how is the policy here?

@mih
Copy link
Member

mih commented Oct 6, 2021

No need to squash, we prefer the full history. thx!

@mih
Copy link
Member

mih commented Oct 6, 2021

All green. Let's go. Thx much @DisasterMo

@mih mih merged commit 588c2c8 into datalad:maint Oct 6, 2021
@DisasterMo DisasterMo deleted the bf-4770-subdatasets-no-suppress branch October 7, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datalad subdatasets output should not just suppress output (or provide info on how to expand)
2 participants