Fix experimental decorator #1248
Conversation
|
@crcrpar Thanks for the review! One question. We currently attach the experimental note to a function and class that do not have a docstring. For these functions and classes, I think just attaching a warning should be enough. What do you think? |
|
That completely makes sense🙂
Though I’m a bit negative about supporting that case if the implementation
gets complicated.
…On Tue, May 19, 2020 at 11:48 Harutaka Kawamura ***@***.***> wrote:
@crcrpar <https://github.com/crcrpar> Thanks for the review! One
question. We currently attach the experimental note to a function and class
that do not have a docstring. For these functions and classes, I think just
attaching a warning should be enough. What do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1248 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3Q7U5QDO7YGNON5UD6RYTRSHXQBANCNFSM4NCYXQHQ>
.
|
|
@harupy The implementation will be like this: ...
@functools.wraps(func)
def new_func(*args, **kwargs):
warnings.warn(...)
return func(*args, **kwargs)
if new_func.__doc__ is None:
return new_func
new_func.__doc__ = ...
return new_func |
Codecov Report
@@ Coverage Diff @@
## master #1248 +/- ##
==========================================
+ Coverage 86.50% 87.05% +0.54%
==========================================
Files 94 94
Lines 7375 7950 +575
==========================================
+ Hits 6380 6921 +541
- Misses 995 1029 +34
Continue to review full report at Codecov.
|
|
That sounds simple enough. It's up to you whether do that in this PR or a following PR. 😄 |
|
@crcrpar I'll open a follow-up. |
|
@harupy Could you fix the |
|
@crcrpar Thank you for the review :) |
|
@toshihikoyanase @hvy Could you please review this PR? |
| def _get_docstring_indent(docstring: str) -> str: | ||
| return docstring.split("\n")[-1] if "\n" in docstring else "" |
There was a problem hiding this comment.
[Question] This function seems to get the last line of the given docstring. Is it always corresponding to an indent?
There was a problem hiding this comment.
Is it always corresponding to an indent?
Not always. There are cases where this function does not work.
# the last line contains characters
def f():
"""summary
detail"""
pass
# the last line has a different indent level
def f():
"""summary
detail
"""
pass
def f():
"""summary
detail
"""
pass
There was a problem hiding this comment.
A not-so-strong drop by comment but I think it's okay assuming https://github.com/optuna/optuna/wiki/Coding-Style-Conventions#docstrings. We could add warnings in case some of these assumptions above are broken but it also sounds like overengineering.
There was a problem hiding this comment.
Thank you for your explanation. Although flake8 and black do not check such inconsistent indent levels, I think we can accept the current implementation as @hvy says.
| def _get_docstring_indent(docstring: str) -> str: | ||
| return docstring.split("\n")[-1] if "\n" in docstring else "" |
There was a problem hiding this comment.
Thank you for your explanation. Although flake8 and black do not check such inconsistent indent levels, I think we can accept the current implementation as @hvy says.
Motivation
Fixes #1213
Description of the changes
Fix
experimentalto decorate a class method properly.