[WIP] Move special-cased types from semanal to a separate module#3134
[WIP] Move special-cased types from semanal to a separate module#3134elazarg wants to merge 19 commits into
Conversation
|
This conflicts with #3081. |
|
@JelleZijlstra I will rebase after #3081 (and other relevant PRs) will be merged. It's only a refactoring so there's no rush. It's here for the feedback. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for cleaning things up! Some feedback based on a quick pass (this is not a full review).
|
|
||
| def calculate_abstract_status(self, typ: TypeInfo) -> None: | ||
| @staticmethod | ||
| def calculate_abstract_status(typ: TypeInfo) -> None: |
There was a problem hiding this comment.
By convention, we usually prefer module-level methods to static methods in the mypy codebase. This would probably be better as a module-level method. There are exceptions such as helper methods that don't make sense outside a particular class, or things that are tightly coupled with the implementations details of a class -- they may be fine as static methods. By default, we use a module-level method if we don't need self.
Rationale:
- Module-level methods make it obvious on the call site that the method doesn't use
self, and without having to use the class prefix. - Module-level methods are easier to move to a different module and reuse, and generally make it a bit easier to refactor code.
- That's how most of the rest of the codebase is written, and consistency is nice.
There was a problem hiding this comment.
You don't need to sell that to me, I completely agree :) I have used staticmethod trying to reduce the footprint of these changes, but this is pretty hopeless (and maybe not desirable, given this is a refactoring PR).
| return self.remove_dups(tvars) | ||
|
|
||
| def remove_dups(self, tvars: List[T]) -> List[T]: | ||
| @staticmethod |
There was a problem hiding this comment.
This should be a module-level method (see above), and most new static methods are probably similar.
| @@ -0,0 +1,912 @@ | |||
| from collections import OrderedDict | |||
|
|
||
|
|
||
| class Special: | ||
| """ |
There was a problem hiding this comment.
Style nit: Add summary of what this class does as the first line of the docstring, followed by an empty line.
| import mypy.semanal | ||
|
|
||
|
|
||
| class Special: |
There was a problem hiding this comment.
Find a better name for the class.
| class Special: | ||
| """ | ||
| Groups special-cased types: | ||
| * NamedTuple |
There was a problem hiding this comment.
I wonder if it would make sense to move the code related to various special cased types to separate classes (or top-level functions), to improve cohesion? They could all live in the same file still. It may make sense to have a single entry point to the module though. (Note that it's probably not worth using inheritance.)
There was a problem hiding this comment.
I wanted this too, but I couldn't decide how to provide the services from SemanticAnalyzer without duplication. Like you say, inheritance (for code reuse) does not sound like a good idea.
I think there are two kinds of services: lookup operations and analysis. It might be possible to refactor the lookup to a separate class (SymbolTable/Scope).
I should give it more thought.
elazarg
changed the title
|
I guess this should wait. |
4 participants
Following @JukkaL comment in #3102.
Along the way I've added some minor unrelated changes, such as turning methods into staticmethods when possible.