Remove redundant lookup funcs in fixup.py #11253
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! We need to be careful with these changes, since there are various subtle differences that could cause (rare) breakage. On the other hand, cleaning these up will make it less likely that things will break in the future.
mypy/lookup.py
Outdated
|
|
||
| def lookup_fully_qualified(name: str, modules: Dict[str, MypyFile], | ||
| raise_on_missing: bool = False) -> Optional[SymbolTableNode]: | ||
| suppress_errors: bool = False) -> Optional[SymbolTableNode]: |
There was a problem hiding this comment.
Negating the meaning of a bool argument can be dangerous, since it will silently change behavior in all call sites that haven't been updated.
I think it's better to leave this unchanged. Instead, you could change mypy/fixup.py to use raise_on_missing instead of suppress_errors for consistency. Changing fixup seems better since it's more of an internal thing.
mypy/fixup.py
Outdated
| info = TypeInfo(SymbolTable(), dummy_def, "<missing>") | ||
| obj_type = lookup_qualified(modules, 'builtins.object', False) | ||
| assert isinstance(obj_type, TypeInfo) | ||
| obj_type = lookup_qualified_typeinfo(modules, 'builtins.object', False) |
There was a problem hiding this comment.
For bool arguments it will be clearer to use a keyword argument instead of positional.
mypy/plugins/attrs.py
Outdated
| # When a converter is set the init_type is overridden by the first argument | ||
| # of the converter method. | ||
| converter = lookup_qualified_stnode(ctx.api.modules, self.converter.name, True) | ||
| converter = lookup_fully_qualified(self.converter.name, ctx.api.modules, True) |
There was a problem hiding this comment.
Nice, it's awkward that this imported a lookup function from mypy.fixup.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! Left a bunch of comments about adding more keyword args to make it explicit whether we are using raise_on_missing or allow_missing.
|
Thanks for reviewing! Making |
2 participants
Description
Removes the redundant lookup funcs in fixup.py. Related to #4157.
lookup_qualified_stnode: uncessary nested call tolookup_fully_qualifiedlookup_qualified: inconsistency return types with otherlookup_qualifiedfuncs. Let the callers extract theSymbolNodefrom theSymbolTableNode.