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: make logic more consistent for files=[] argument (which is False but not None) #6976

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

yarikoptic
Copy link
Member

Similar problem was resolved previously at higher level in
http://github.com/datalad/datalad/commit/ec0243c92822f36ada5e87557eb9f5f53929c9ff
for get_content_info. But IMHO it could and should be resolved at the
lowest/common point of invocation, that is why I added it to
_generator_call_git which is underlying most (if not all, hard to tell now ;) )
git invocations.

I also fixed a few other invocations where it was pure if files or if paths.

This commit was initially proposed as a part of the #6974 but there it failed and later I realized that approach I proposed in that PR for get_modules_ was flawed, so this PR is dedicated to only this change (although at once across different commands/levels). Want to see if we could distill it independently.

…but not None)

Similar problem was resolved previously at higher level in
http://github.com/datalad/datalad/commit/ec0243c92822f36ada5e87557eb9f5f53929c9ff
for get_content_info.  But IMHO it could and should be resolved at the
lowest/common point of invocation, that is why I added it to
_generator_call_git which is underlying most (if not all, hard to tell now ;) )
git invocations.

I also fixed a few other invocations where it was pure `if files` or `if paths`
instead of correctly checking against None.
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #6976 (f7734c6) into maint (29b42b4) will increase coverage by 0.47%.
The diff coverage is 100.00%.

❗ Current head f7734c6 differs from pull request most recent head a6814ba. Consider uploading reports for the commit a6814ba to get more accurate results

@@            Coverage Diff             @@
##            maint    #6976      +/-   ##
==========================================
+ Coverage   90.13%   90.61%   +0.47%     
==========================================
  Files         354      354              
  Lines       46326    46328       +2     
  Branches     6613        0    -6613     
==========================================
+ Hits        41755    41978     +223     
+ Misses       4554     4350     -204     
+ Partials       17        0      -17     
Impacted Files Coverage Δ
datalad/dataset/gitrepo.py 96.84% <100.00%> (+0.02%) ⬆️
datalad/support/annexrepo.py 90.35% <100.00%> (-0.17%) ⬇️
datalad/support/gitrepo.py 91.02% <100.00%> (-1.72%) ⬇️
datalad/cmdline/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/run_procedure.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/cmdline/helpers.py 0.00% <0.00%> (-78.27%) ⬇️
datalad/support/nda_.py 0.00% <0.00%> (-54.55%) ⬇️
datalad/__main__.py 39.65% <0.00%> (-29.32%) ⬇️
datalad/support/collections.py 84.37% <0.00%> (-9.38%) ⬇️
datalad/distribution/create_sibling.py 65.24% <0.00%> (-4.56%) ⬇️
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Aug 24, 2022
@yarikoptic
Copy link
Member Author

wow -- "all green" . The only confusing item is a flaky test failure on Travis - confused since I thought that we xfail'ed that test_ExtractedArchive recently in https://github.com/datalad/datalad/pull/6912/files . And code in traceback shows that try but not clear how the heck if 'nfsmount' in fpath: did not match or pytest.xfail( didn't raise its own exception? I dont' know. I have restarted that job. And will take this PR our of draft since I think it is the right thing to do/fix here although we might run into cases where it would show changed behavior (but may be "for good")

@yarikoptic yarikoptic marked this pull request as ready for review August 24, 2022 15:51
@yarikoptic yarikoptic added the release Create a release when this pr is merged label Aug 30, 2022
@yarikoptic
Copy link
Member Author

eh, last commit adjusting for "early decision making" did introduce regression!

        assert_false(
            ds.repo.get_content_annexinfo(paths=[], ref="HEAD", init=None))
        # ... where whatever was passed for init will be returned as is.
>       assert_equal(
            ds.repo.get_content_annexinfo(
                paths=[], ref="HEAD", init={"random": {"entry": "a"}}),
            {"random": {"entry": "a"}})
../datalad/support/tests/test_fileinfo.py:323: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
first = OrderedDict(), second = {'random': {'entry': 'a'}}, msg = None
    def assert_equal(first, second, msg=None):
        if msg is None:
>           assert first == second
E           AssertionError: assert OrderedDict() == {'random': {'entry': 'a'}}
E             Right contains 1 more item:
E             {'random': {'entry': 'a'}}
E             Full diff:
E             - {'random': {'entry': 'a'}}
E             + OrderedDict()
../datalad/tests/utils_pytest.py:107: AssertionError

because there is apparently some extra semantic of init for dict-like.

        init : 'git' or dict-like or None
          If set to 'git' annex content info will amend the output of
          GitRepo.get_content_info(), otherwise the dict-like object
          supplied will receive this information and the present keys will
          limit the report of annex properties. Alternatively, if `None`
          is given, no initialization is done, and no limit is in effect.

which is imho likely something to be done outside or with a helper... Hence I reverted that shortcut return change, leaving only the portion for that "heavily debated" if paths to stay as is.

@yarikoptic
Copy link
Member Author

appveyor fail is unrelated

FAILED ../datalad/support/tests/test_parallel.py::test_gracefull_death - assert 20 >= 23

Let's proceed with the release and someone could subsequently push on more of similar unification or adding shortcut returns for some performance benefit.

@yarikoptic yarikoptic merged commit 870c72a into datalad:maint Aug 30, 2022
@yarikoptic yarikoptic deleted the bf-68849-ext branch August 30, 2022 18:49
@github-actions
Copy link

🚀 PR was released in 0.17.4 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged released semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant