Don't require a returns section for NoReturn
#57
Conversation
When a function has a return type of `NoReturn` it means it will never return, so the documentation doesn't really need to specify a return. The option `--require-return-section-when-returning-none` was renamed to `--require-return-section-when-returning-nothing` to highlight it is more generic now, as it will also require a returns section for `NoReturn`. The old option is kept as a deprecation error. Signed-off-by: Leandro Lucarella <[email protected]>
| return returnAnnotation.startswith('NoReturn') | ||
|
|
||
|
|
||
| def _isNoReturn(node: ast.AST) -> bool: |
There was a problem hiding this comment.
This function doesn't seem to be used anywhere?
There was a problem hiding this comment.
And comparing the implementation of this function and that of isReturnAnnotationNoReturn(), it seems that the implementation of this function is better?
There was a problem hiding this comment.
Hi @jsh9!
This function doesn't seem to be used anywhere?
Yeah, good catch. This is a leftover from a version that didn't work. I should (maybe?) remove it.
And comparing the implementation of this function and that of
isReturnAnnotationNoReturn(), it seems that the implementation of this function is better?
It looks better but it didn't work for me. I'm not familiar with the ast stuff here, so not sure if there is a way to make it work, but when I tried it it seems like NoReturn is not a ast.Constant but a ast.Name IIRC., this is why I had to go with the string comparison. I think I could also just do returnAnnotation == 'NoReturn', the startwidth() was based on the generator stuff but in this case I think it should only match to NoReturn.
|
Hi @llucax , thank you so much for finding this issue and submitting this PR! The code changes generally look good, and I think the renaming from "none" and "nothing" is very appropriate! I left a few comments above, basically some curious questions. Would you mind taking a look at them? |
This applies the suggestion made during the review. Co-authored-by: jsh9 <[email protected]>
Also compare against `NoReturn` via `==` instead of using `startswith()`, as we really want to match `NoReturn` literally. Signed-off-by: Leandro Lucarella <[email protected]>
|
Hi @jsh9, I updated the PR with your suggestions and comments. Let me know if you think |
|
Hi @llucax , I think Thanks again for the contribution! |
When a function has a return type of
NoReturnit means it will never return, so the documentation doesn't really need to specify a return.The option
--require-return-section-when-returning-nonewas renamed to--require-return-section-when-returning-nothingto highlight it is more generic now, as it will also require a returns section forNoReturn. The old option is kept as a deprecation error.Fixes #55.