-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[clean strict optional] Fix some strict optional errors in mypy #3228
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
Conversation
@@ -105,7 +106,7 @@ def parse_conversion_specifiers(self, format: str) -> List[ConversionSpecifier]: | |||
return specifiers | |||
|
|||
def analyze_conversion_specifiers(self, specifiers: List[ConversionSpecifier], | |||
context: Context) -> bool: | |||
context: Context) -> Optional[bool]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below distinguishes False
and None
@@ -304,7 +304,7 @@ def __init__(self, reports: Reports, output_dir: str) -> None: | |||
self.css_html_path = os.path.join(reports.data_dir, 'xml', 'mypy-html.css') | |||
xsd_path = os.path.join(reports.data_dir, 'xml', 'mypy.xsd') | |||
self.schema = etree.XMLSchema(etree.parse(xsd_path)) | |||
self.last_xml = None # type: etree._ElementTree | |||
self.last_xml = None # type: Optional[etree._ElementTree] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it wouldn't be better to keep this as non-optional and remove the None assignments? Not sure I follow the logic there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the logic correctly, those None
assignments are needed, but the attribute should be always non-None
at the end, so that I replaced if
s with asserts, I checked locally it works correctly.
mypy/strconv.py
Outdated
@@ -24,10 +24,9 @@ class StrConv(NodeVisitor[str]): | |||
|
|||
def __init__(self, show_ids: bool = False) -> None: | |||
self.show_ids = show_ids | |||
self.id_mapper = None # type: IdMapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Optional? (Currently mypy ignores the None initialization but we may change that, and it seems to me this function can definitely return with id_mapper
still None.
mypy/stubutil.py
Outdated
@@ -90,7 +90,8 @@ def is_c_module(module: ModuleType) -> bool: | |||
return '__file__' not in module.__dict__ or module.__dict__['__file__'].endswith('.so') | |||
|
|||
|
|||
def write_header(file: IO[str], module_name: str, pyversion: Tuple[int, int] = (3, 5)) -> None: | |||
def write_header(file: IO[str], module_name: str = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer Optional[str], EIBTI. (I somewhat regret the rule in PEP 484 about this, or at least I don't enjoy relying on it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will use Optional[X]
(also for previous comment) in this and next PRs. I don't have a preference concerning "syntactic sugar" just would prefer this to be consistent in function annotations and variable annotations (i.e. either both or neither).
I will implement your comments tomorrow, sorry don't have time today.
Thanks! Let's remember to mark these files with |
This PR fixes 26
--strict-optional
errors in mypy itself (out of 547 total) in following files: