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

Correct decorator implementation using the wrapt library #2991

Merged
merged 1 commit into from
Jun 16, 2019

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 11, 2019

Fixes #2446

The naive implementation of a decorator will break some of the
functionality of the functions they wrap. For example, the original
docstring will be lost. The module functools provides the wraps
decorator to fix this problem, but this is not the only issue. In
addition to the docstring, other properties, such as the function
signature are also lost. This means that using the inspect module on
decorated functions is hopelessly broken. The wrapt library provides
the decorator decorator that simplifies writing decorators, while
ensuring that all the important properties of wrapped function are
maintained.

@sphuber sphuber requested review from ltalirz and giovannipizzi June 11, 2019 17:30
@sphuber
Copy link
Contributor Author

sphuber commented Jun 11, 2019

A few notes:

  1. I have not adapted the implementation of the protected nor override decorators yet because they are a bit more complicated.
  2. The same goes for the calcfunction and workfunction decorator
  3. The with_dbenv currently does not take an argument, so could be simplified by removing a wrapping layer. However, this will require it to be used as @with_dbenv instead of @with_dbenv(). Since this is a breaking change, I have left it. Moreover, if in the future we want to allow to specify which profile to load through the decorator, this can be done without breaking the interface, I think

Before I try to see if I can tackle 1 and 2, I wanted to see if you guys feel this is worth the added dependency. I certainly do in any case.

@sphuber sphuber force-pushed the fix_2446_wrapt_decorators branch from 453f99b to e816c04 Compare June 11, 2019 17:47
@coveralls
Copy link

coveralls commented Jun 11, 2019

Coverage Status

Coverage increased (+0.05%) to 73.217% when pulling b28642a on sphuber:fix_2446_wrapt_decorators into 9abc4ff on aiidateam:develop.

@ltalirz
Copy link
Member

ltalirz commented Jun 11, 2019

wrapt seems like a nice library, and it's available on conda as well.
the only criticism I could have here is that it seems to have a bus factor of 1 and that I don't consider the features we add here terribly important - I agree that they are nice to have, though, and would be ok with including wrapt as a dependency.

@giovannipizzi
Copy link
Member

Same comment as @ltalirz - anyway it seems that the library is quite stable and only doing bugfixes in the past two years.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

fine to go ahead!

The naive implementation of a decorator will break some of the
functionality of the functions they wrap. For example, the original
docstring will be lost. The module `functools` provides the `wraps`
decorator to fix this problem, but this is not the only issue. In
addition to the docstring, other properties, such as the function
signature are also lost. This means that using the `inspect` module on
decorated functions is hopelessly broken. The `wrapt` library provides
the `decorator` decorator that simplifies writing decorators, while
ensuring that all the important properties of wrapped function are
maintained.
@sphuber sphuber force-pushed the fix_2446_wrapt_decorators branch from e816c04 to b28642a Compare June 15, 2019 21:57
@sphuber sphuber merged commit 961e745 into aiidateam:develop Jun 16, 2019
@sphuber sphuber deleted the fix_2446_wrapt_decorators branch June 16, 2019 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current implementation of decorators loses function signature
4 participants