Skip to content
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

Remove more redundant casts #1699

Merged
merged 2 commits into from
Jun 15, 2016
Merged

Conversation

rwbarton
Copy link
Contributor

No description provided.

@rwbarton
Copy link
Contributor Author

The cast in typeanal.py is redundant for a fairly non-obvious reason; it's redundant only because each of the branches of the preceding 65-line if/else-chain exits with a return and the last condition is not isinstance(sym.node, TypeInfo). I don't know what a good way to handle this is though. Writing a # type: TypeInfo comment doesn't quite work because there is a previous definition of info (though that one could just be renamed).

@gnprice
Copy link
Collaborator

gnprice commented Jun 14, 2016

Looks good -- thanks!

In the typeanal.py case, I think in fact that previous info definition:

             elif fullname == 'typing.Tuple':
                 if len(t.args) == 2 and isinstance(t.args[1], EllipsisType):
                     # Tuple[T, ...] (uniform, variable-length tuple)
                     node = self.lookup_fqn_func('builtins.tuple')
                     info = cast(TypeInfo, node.node)
                     return Instance(info, [t.args[0].accept(self)], t.line)

would probably be better named something like tuple_info in any case. Generally the other locals with names like t, sym, fullname, tvar_expr, etc., that are just named after a kind of thing refer to that kind of thing as derived in a canonical way from the argument t that we're visiting and the symbol bound to it. This one is not derived from t, but rather we went and got builtins.tuple off the shelf because t turned out to refer to a particular kind of tuple type.

I think this overall chain of conditional logic could also be made clearer -- in particular, I get uneasy reading it because it's not made totally manifest that all the conditions are mutually exclusive, and if they aren't there's no indication of why the order they're in should be the right one. For example, two conditions check sym.kind, one after all the fullname checks and the other before. I suspect the conditions actually are mutually exclusive, but it'd be very reassuring as a reader for that to be explicit.

@rwbarton
Copy link
Contributor Author

In the typeanal.py case, I think in fact that previous info definition:

         elif fullname == 'typing.Tuple':
             if len(t.args) == 2 and isinstance(t.args[1], EllipsisType):
                 # Tuple[T, ...] (uniform, variable-length tuple)
                 node = self.lookup_fqn_func('builtins.tuple')
                 info = cast(TypeInfo, node.node)
                 return Instance(info, [t.args[0].accept(self)], t.line)

would probably be better named something like tuple_info in any case.

That was my feeling too, so I renamed it and added the type annotation to info below.

@rwbarton
Copy link
Contributor Author

BTW, in this case the timing was a coincidence, but my preference would be to do this sort of cleanup immediately after each release (if applicable). My workflow involves using mypy mypy a lot and I install each new release of mypy so it helps me if mypy HEAD at least mostly continues to type check using the latest released version of mypy.

@rwbarton rwbarton merged commit a3f002f into python:master Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants