Repository navigation
Proposal: Refactor extract class AssignmentStatementAnalyzer from SemanticAnalyzer #2198
Description
I want to propose a refactoring that I will volunteer to execute. Before spending time on it, I would like to see if there are any high-level objections.
Implementation Proposal
(1) The following tree of methods will be extracted from SemanticAnalyzer and moved to a new class _AssignmentStatementAnalyzer that is used internally by SemanticAnalyzer.
visit_assignment_stmt (public)
analyze_simple_literal_type
check_and_set_up_type_alias
process_newtype_declaration
analyze_newtype_declaration
check_newtype_args
build_newtype_typeinfo
make_argument
process_typevar_declaration
check_typevar_name
get_typevar_declaration
analyze_types
process_typevar_parameters
process_namedtuple_definition
check_namedtuple
parse_namedtuple_args
parse_namedtuple_fields_with_types
fail_namedtuple_arg
build_namedtuple_typeinfo
All methods in the new _AssignmentStatementAnalyzer class except visit_assignment_stmt() will be prefixed with underscore to indicate that they are private to the class.
(2) SemanticAnalyzer.visit_assignment_stmt() will be altered to delegate to _AssignmentStatementAnalyzer.visit_assignment_stmt().
Benefits
- Total surface area of SemanticAnalyzer is reduced by:
- 582 lines of code -- 25% of the original 2424 LOC in SemanticAnalyzer
- 18 methods
- Public surface area of the extracted method group is reduced to 1 method:
- visit_assignment_stmt()
I believe that this change will significantly improve the readability of SemanticAnalyzer for future readers.
Activity
gvanrossum commentedon Sep 30, 2016
davidfstr commentedon Sep 30, 2016
Okay then I won't volunteer to take this task on myself at the present time.
Indeed there will be some. For any potential implementors who may be reading, one detail I left out is that that a small number of private trampoline methods will need to be added to _AssignmentStatementAnalyzer to assist in forwarding requests to the original SemanticAnalyzer that aren't handled internally by _AssignmentStatementAnalyzer.
If there are no other immediate implementor volunteers then I suggest closing this issue.
elazarg commentedon Dec 12, 2016
This refactoring is needed, although it's not exactly about AssignmentStatement (a nametuple might be a base class expression). Personally I'd move everything to a different module, not just a class - and perhaps no class is needed at all. This is of course related to #699.
I have actually done something very similar in a private branch, and the back-and-forth weren't such uneasy. Errors are reported using a simple callback or return values; there are very few dependencies otherwise.
Note that there are two independent tasks here: the parsing of "external" language constructs, and the semantic analysis of these constructs. The first task should IMO done before semantic analysis, as a pre-parsing phase.