Skip to content

Added use of @functools.wraps() in our decorators#283

Merged
tleonhardt merged 1 commit intomasterfrom
functools_wraps
Feb 27, 2018
Merged

Added use of @functools.wraps() in our decorators#283
tleonhardt merged 1 commit intomasterfrom
functools_wraps

Conversation

@tleonhardt
Copy link
Copy Markdown
Member

This updates our decorator wrapper functions to look like the wrapped function.

This partially addresses Issue #271 where using multiple decorators can break help on subcommands, depending on the order in which the decorators are applied.

This PR doesn't actually fix anything in and of itself. But it encourages the usage of @functools.wraps(). If other decorators use this, then there shouldn't be any problem. Of course, we can't control how 3rd-party libraries implement decorators. But we can at least be part of the solution instead of part of the problem.

This updates a wrapper function to look like the wrapped function.

This partially addresses Issue #271 where using multiple decorators can break help on subcommands, depending on the order in which the decorators are applied.

This PR doesn't actually fix anything in and of itself.  But it encourages the usage of @functools.wraps().  If other decorators use this, then there shouldn't be any problem.  Of course, we can't control how 3rd-party libraries implement decorators.  But we can at least be part of the solution instead of part of the problem.
@tleonhardt tleonhardt added this to the 0.8.1 milestone Feb 24, 2018
@tleonhardt tleonhardt requested a review from kotfu February 24, 2018 04:16
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   91.56%   91.57%   +<.01%     
==========================================
  Files           1        1              
  Lines        1423     1424       +1     
==========================================
+ Hits         1303     1304       +1     
  Misses        120      120
Impacted Files Coverage Δ
cmd2.py 91.57% <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 656a774...c40a962. Read the comment docs.

@tleonhardt
Copy link
Copy Markdown
Member Author

@kotfu Is there more you would like to do for #271? Or should this PR close it?

Copy link
Copy Markdown
Contributor

@kotfu kotfu left a comment

Choose a reason for hiding this comment

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

This looks good. I guess we'll just have to hope everyone else is a good citizen.

@tleonhardt tleonhardt merged commit 41c5484 into master Feb 27, 2018
@tleonhardt tleonhardt deleted the functools_wraps branch February 27, 2018 13:49
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.

2 participants