Skip to content

Conversation

@kmvanbrunt
Copy link
Member

Fixes #633

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

codecov bot commented Mar 4, 2019

Codecov Report

Merging #634 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
+ Coverage   94.08%   94.08%   +<.01%     
==========================================
  Files          10       10              
  Lines        2957     2959       +2     
==========================================
+ Hits         2782     2784       +2     
  Misses        175      175
Impacted Files Coverage Δ
cmd2/cmd2.py 94.52% <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 de70108...95d2d47. Read the comment docs.

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.

Thanks for finding and fixing this bug. Shame on me for not creating a relevant unit test in the first place!

Your changes look good, but please take a look at my comments.

Feedback from @kotfu would also be appreciated - he may have an interesting decorator trick up his sleeve that could get around the required parentheses.

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.

I'd recommend keeping the relevant comment in the docs and just updating it accordingly.

If we can find some intelligent way of not requiring the parentheses, then that would be great.

arg 2: 'bar'
arg 3: 'baz'

.. note::
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend keeping this note and just updating it to show the required parentheses.

@tleonhardt tleonhardt added the bug label Mar 4, 2019
@tleonhardt tleonhardt added this to the 0.9.11 milestone Mar 4, 2019
@tleonhardt
Copy link
Member

See this StackOverflow post for a possible way we could use the decorator with or without parents: https://stackoverflow.com/questions/35572663/using-python-decorator-with-or-without-parentheses

@kmvanbrunt kmvanbrunt closed this Mar 4, 2019
@kmvanbrunt kmvanbrunt deleted the fix_with_argument_list branch March 4, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants