Skip to content

Conversation

@kmvanbrunt
Copy link
Member

@kmvanbrunt kmvanbrunt commented Mar 1, 2019

Removed ability to call commands in pyscript as if they were functions (e.g app.help()) in favor of only supporting one pyscript interface. This simplifies future maintenance.

This closes #586 since the feature will no longer exist.

@kmvanbrunt kmvanbrunt self-assigned this Mar 1, 2019
@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #630 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #630      +/-   ##
==========================================
- Coverage   94.13%   94.08%   -0.05%     
==========================================
  Files          10       10              
  Lines        3101     2957     -144     
==========================================
- Hits         2919     2782     -137     
+ Misses        182      175       -7
Impacted Files Coverage Δ
cmd2/utils.py 98.63% <ø> (ø) ⬆️
cmd2/pyscript_bridge.py 100% <100%> (+2.19%) ⬆️
cmd2/cmd2.py 94.52% <100%> (+0.16%) ⬆️
cmd2/argparse_completer.py 87.88% <100%> (ø) ⬆️

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 d3208c8...daf5e2b. Read the comment docs.

@tleonhardt tleonhardt added this to the 0.9.11 milestone Mar 1, 2019
Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

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

Overall it looks good. Please look at my comments.

@@ -1,3 +0,0 @@
# flake8: noqa F821
Copy link
Member

Choose a reason for hiding this comment

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

Is deleting the majority of our pyscript unit tests the right thing to do? Or would it be better to convert them to using the other syntax given that is the only supported one now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The deleted tests only exercised the ArgparseFunctor stuff.

The remaining interface to pyscript is simple because it does no translation before sending the command to our parser. It is pretty well covered by existing unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe take a quick look at code coverage within do_pyscript and pyscript_bridge. But I think your current approach looks OK overall.

@tleonhardt
Copy link
Member

@kmvanbrunt Would it be reasonable to address #613 in this PR too?

@kmvanbrunt kmvanbrunt merged commit de70108 into master Mar 1, 2019
@kmvanbrunt kmvanbrunt deleted the pyscript_overhaul branch March 1, 2019 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

py app.command() doesn't work if command has sub-commands

3 participants