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

RF: use parse_known_args instead of _parse_known_args #6414

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

yarikoptic
Copy link
Member

parse_known_args seems to be doing some processing which we did not
apparently needed. @mih raised concern/question on why we use protected
method and there were no comment/rationale left in the code.

I have set exit_on_error to False since we do not want to add that
error handling behavior since we did not rely on it before

Timing on my laptop on this commit

$> multitime -n 5 datalad --help-np 1>/dev/null
===> multitime results
1: datalad --help-np
			Mean        Std.Dev.    Min         Median      Max
real        1.304       0.026       1.276       1.294       1.353
user        1.232       0.035       1.197       1.221       1.300
sys         0.076       0.010       0.057       0.080       0.085

and on master

$> multitime -n 5 datalad --help-np 1>/dev/null
===> multitime results
1: datalad --help-np
			Mean        Std.Dev.    Min         Median      Max
real        1.312       0.037       1.270       1.315       1.365
user        1.229       0.032       1.184       1.228       1.265
sys         0.087       0.012       0.072       0.090       0.106

so no notable negative impact (good).

🏠 Internal

  • Use public ArgumentParser.parse_known_args instead of protected _parse_known_args

parse_known_args seems to be doing some processing which we did not
apparently needed. @mih raised concern/question on why we use protected
method and there were no comment/rationale left in the code.

I have set exit_on_error to False since we do not want to add that
error handling behavior since we did not rely on it before

Timing on my laptop on this commit

	$> multitime -n 5 datalad --help-np 1>/dev/null
	===> multitime results
	1: datalad --help-np
				Mean        Std.Dev.    Min         Median      Max
	real        1.304       0.026       1.276       1.294       1.353
	user        1.232       0.035       1.197       1.221       1.300
	sys         0.076       0.010       0.057       0.080       0.085

and on master

	$> multitime -n 5 datalad --help-np 1>/dev/null
	===> multitime results
	1: datalad --help-np
				Mean        Std.Dev.    Min         Median      Max
	real        1.312       0.037       1.270       1.315       1.365
	user        1.229       0.032       1.184       1.228       1.265
	sys         0.087       0.012       0.072       0.090       0.106

so no notable negative impact (good).
@yarikoptic
Copy link
Member Author

aha -- failures against extensions shine the light on "why/whatfor":

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.[7](https://github.com/datalad/datalad/runs/5053410484?check_suite_focus=true#step:11:7).12/x64/bin/datalad", line [8](https://github.com/datalad/datalad/runs/5053410484?check_suite_focus=true#step:11:8), in <module>
    sys.exit(main())
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/datalad/cli/main.py", line 74, in main
    parser = setup_parser(args, completing="_ARGCOMPLETE" in os.environ)
  File "/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/datalad/cli/parser.py", line [10](https://github.com/datalad/datalad/runs/5053410484?check_suite_focus=true#step:11:10)4, in setup_parser
    exit_on_error=False,
TypeError: __init__() got an unexpected keyword argument 'exit_on_error'
Error: Process completed with exit code 1.

looking at http://github.com/python/cpython/blob/HEAD/Doc/library/argparse.rst

   .. versionchanged:: 3.9
      *exit_on_error* parameter was added.

so we could do the switch starting from 3.9. I will add a commit reverting this RF and adding a comment to do whenever 3.9 is minimal version.

@codeclimate
Copy link

codeclimate bot commented Feb 3, 2022

Code Climate has analyzed commit 03645e5 and detected 0 issues on this pull request.

View more on Code Climate.

@yarikoptic yarikoptic added the semver-internal Changes only affect the internal API label Feb 3, 2022
@yarikoptic
Copy link
Member Author

@jwodder is there a way to instruct auto to not bother about changelog entry at all for a PR?

@jwodder
Copy link
Member

jwodder commented Feb 3, 2022

@yarikoptic I don't think so.

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #6414 (03645e5) into master (cc84266) will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6414      +/-   ##
==========================================
+ Coverage   89.57%   90.01%   +0.43%     
==========================================
  Files         345      345              
  Lines       43032    43312     +280     
==========================================
+ Hits        38545    38986     +441     
+ Misses       4487     4326     -161     
Impacted Files Coverage Δ
datalad/cli/parser.py 84.76% <ø> (+0.47%) ⬆️
datalad/_version.py 46.99% <0.00%> (-53.01%) ⬇️
datalad/customremotes/tests/test_archives.py 89.28% <0.00%> (-0.65%) ⬇️
datalad/support/tests/test_annexrepo.py 97.97% <0.00%> (+0.13%) ⬆️
datalad/support/gitrepo.py 90.37% <0.00%> (+0.16%) ⬆️
datalad/distribution/tests/test_install.py 100.00% <0.00%> (+0.20%) ⬆️
datalad/distribution/tests/test_dataset.py 99.70% <0.00%> (+0.29%) ⬆️
datalad/core/distributed/tests/test_clone.py 97.57% <0.00%> (+0.36%) ⬆️
datalad/local/add_archive_content.py 88.03% <0.00%> (+0.42%) ⬆️
datalad/support/network.py 87.22% <0.00%> (+0.44%) ⬆️
... and 40 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 cc84266...03645e5. Read the comment docs.

@mih mih added the team-core core API/commands (https://github.com/datalad/datalad/issues/6365) label Feb 7, 2022
@mih
Copy link
Member

mih commented Feb 7, 2022

Thx for the investigation and leaving this note!

@mih mih merged commit 6dc0c58 into datalad:master Feb 7, 2022
@yarikoptic yarikoptic deleted the rf-parse_known_args branch April 5, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API team-core core API/commands (https://github.com/datalad/datalad/issues/6365)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants