update: forcing explicit optional types #17
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 326 339 +13
Branches 97 103 +6
=========================================
+ Hits 326 339 +13
☔ View full report in Codecov by Sentry. |
| @@ -87,6 +87,7 @@ class _Visitor(ast.NodeVisitor): | |||
| "BCS020": "Function '%s' has return documentation but no return type.", | |||
| "BCS021": "Function '%s' is missing return documentation.", | |||
| "BCS022": "Found '%d' invalid indents starting with line ('%s').", | |||
| "BCS023": "Argument '%s' is optional but type hint doesn't end with '| None'.", | |||
There was a problem hiding this comment.
I'd prefer we avoid "optional" here as it is ambiguous and doesn't exactly align with the requirements. bool = False or str = '' are both optional args that don't include None in their type. It's an existing pain point that Optional[...] doesn't align with whether an arg is "optional", so might as well use clearer terms
There was a problem hiding this comment.
Good call. Will change.
| and isinstance(default_value, ast.Constant) | ||
| and default_value.value is None | ||
| ): | ||
| if not documented_type.endswith("|None") and not documented_type.startswith("Optional"): |
There was a problem hiding this comment.
Is there a way to do this through attributes without string processing? Seems this allows str | None and Optional[str], but what about Union[str, None] or None | str?
There was a problem hiding this comment.
I think it makes sense to standardize on putting | None last, and we'll see if we need to deal with edge cases for Union.
There was a problem hiding this comment.
Actually checking through the types would be better, but it would mean processing them twice or refactoring around how we deal with types. I'm not sure that it's worth it at this point.
| Returns: | ||
| int: value of the function | ||
| """ | ||
| pass |
Issue #, if available:
Description of changes:
Testing done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.