Skip to content

BF: if early attempt to parse resulted in error, setup subparsers#6980

Merged
yarikoptic merged 1 commit into
datalad:maintfrom
yarikoptic:bf-cli-parser-dashopt
Aug 29, 2022
Merged

BF: if early attempt to parse resulted in error, setup subparsers#6980
yarikoptic merged 1 commit into
datalad:maintfrom
yarikoptic:bf-cli-parser-dashopt

Conversation

@yarikoptic

@yarikoptic yarikoptic commented Aug 24, 2022

Copy link
Copy Markdown
Member

Without that using a short option for a command which is also
present on top level results in early attempt error and then no
parsers even tried, thus giving that error from the main/top parser to the user

Fixes #6814

Regression was introduced in 0b4937d ( AKA 0.16.0^268^217 )

I have not investigated possible impact on completion etc, but when tried - still worked fine.

@yarikoptic yarikoptic added the UX user experience label Aug 24, 2022
@yarikoptic yarikoptic requested a review from mih August 24, 2022 18:00
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Aug 24, 2022
@yarikoptic

yarikoptic commented Aug 24, 2022

Copy link
Copy Markdown
Member Author

wow this hit some nerve:

FAILED ../datalad/cli/tests/test_main.py::test_script_shims[datalad] - datala...
FAILED ../datalad/cli/tests/test_main.py::test_script_shims[git-annex-remote-datalad-archives]
FAILED ../datalad/cli/tests/test_main.py::test_script_shims[git-annex-remote-datalad]
FAILED ../datalad/cli/tests/test_main.py::test_cfg_override - FileNotFoundErr...
FAILED ../datalad/cli/tests/test_main.py::test_librarymode - datalad.runner.e...
FAILED ../datalad/cli/tests/test_main.py::test_completion - datalad.runner.ex...
FAILED ../datalad/core/distributed/tests/test_clone.py::test_invalid_args - d...
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_crcns - Fi...
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_datasets_root
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_simple_local
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_dataset_from_just_source
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_isnot_recursive
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_into_dataset
FAILED ../datalad/core/distributed/tests/test_clone.py::test_notclone_known_subdataset
FAILED ../datalad/core/distributed/tests/test_clone.py::test_clone_missing_commit
FAILED ../datalad/core/distributed/tests/test_clone.py::test_reckless[True]
FAILED ../datalad/core/distributed/tests/test_clone.py::test_reckless[False]
FAILED ../datalad/core/distributed/tests/test_clone.py::test_install_source_relpath

I guess I or someone else would need to look into it in greater depth :-/

edit : with a sober eye -- all those FAILs simply can't relate to the change since have nothing to do with the parser -- what was going on?

@yarikoptic yarikoptic force-pushed the bf-cli-parser-dashopt branch from ded5eec to 3a2f66f Compare August 25, 2022 00:29
@mih

mih commented Aug 25, 2022

Copy link
Copy Markdown
Member

I triggered a rerun of the failed appveyor runs to get a sense of whether the widespread errors are a consequence of some service failure.

Re fix: Superficially, this makes sense. Even though I did the last RF of this code, it is hard for me to say "OK" with confidence. The last RF was focused on making things faster, but it certainly did not aim for (or achieve) an improvement of this code's comprehensibility.

@yarikoptic

Copy link
Copy Markdown
Member Author

bad news: somehow all those FAILED are "real". good news: python -m pytest -s -v -k 'test_main' seems to reproduce one of the failures in test_cfg_override for me locally (but not if I just run -k test_cfg_override -- so smells of some side effect somewhere). I will try to look into it later (on the move in the coming hours)

@yarikoptic yarikoptic force-pushed the bf-cli-parser-dashopt branch from 3a2f66f to a7ca741 Compare August 25, 2022 11:54
Without that using a short option for a command which is also
present on top level results in early attempt error and then no
parsers even tried, thus giving that error from the main/top parser to the user

Fixes 6814

Regression was introduced in 0b4937d
@yarikoptic yarikoptic force-pushed the bf-cli-parser-dashopt branch from a7ca741 to 9b21010 Compare August 25, 2022 11:56
@yarikoptic

Copy link
Copy Markdown
Member Author

gy gy - it was the new test I added. I didn't realize that run_main does it in the process and doesn't run external datalad, so that -C I added was chdir'ing test process and causing all this turmoil. I rewrote and force pushed (twice ;-) ). Should be all good/only relevant fails now ;)

@codecov

codecov Bot commented Aug 25, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6980 (9b21010) into maint (3300769) will increase coverage by 0.99%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6980      +/-   ##
==========================================
+ Coverage   90.14%   91.13%   +0.99%     
==========================================
  Files         354      354              
  Lines       46321    46325       +4     
  Branches     6612     6612              
==========================================
+ Hits        41754    42218     +464     
+ Misses       4551     4091     -460     
  Partials       16       16              
Impacted Files Coverage Δ
datalad/cli/parser.py 86.03% <ø> (+1.80%) ⬆️
datalad/cli/tests/test_main.py 95.34% <100.00%> (+0.11%) ⬆️
datalad/_version.py 45.68% <0.00%> (-0.28%) ⬇️
datalad/cli/interface.py 100.00% <0.00%> (+2.77%) ⬆️
datalad/tests/utils.py 65.10% <0.00%> (+11.03%) ⬆️
datalad/tests/test_tests_utils.py 92.34% <0.00%> (+92.34%) ⬆️

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

@yarikoptic

Copy link
Copy Markdown
Member Author

the only fail travis is a known #4496 (comment) . As @mih gives some support in the fix ("makes sense") , we have a unittest for it now, and it seems nothing breaks -- I will proceed with merge since unlikely we would get a better fix (if there is one possible) any time soon .

@yarikoptic yarikoptic merged commit ebbe341 into datalad:maint Aug 29, 2022
@github-actions

Copy link
Copy Markdown
Contributor

🚀 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

released semver-patch Increment the patch version when merged UX user experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants