Open JukkaL opened 5 years ago
I think 85 is a good number. Are we going to also count comments? Some overload-related functions contain several detailed comments, which makes them visually long, but conceptually simple.
This wouldn't be a hard rule. If very long comments make the function easier to understand, they may be fine. But I think that comments and docstrings should generally be included in the line count, since they can result in the function being too long to be viewed without scrolling.
OK, I agree.
FWIW, here are functions with ~80 lines or more that I could find (the numbers are approximate):
Path and function name Lines
----------------------------------------------------------------------
mypy/main.py:process_options 515
mypy/subtypes.py:is_callable_compatible 277
mypy/checkmember.py:analyze_member_access 201
mypy/typeanal.py:visit_unbound_type_nonoptional 199
mypy/checkexpr.py:check_op_reversible 194
mypy/meet.py:is_overlapping_types 188
mypy/checker.py:check_func_def 178
mypy/options.py:__init__ 164
mypy/semanal.py:_visit_overloaded_func_def 152
mypy/messages.py:format_bare 152
mypy/checkexpr.py:check_call 141
mypy/build.py:process_graph 140
mypy/checker.py:find_isinstance_check 138
mypy/messages.py:incompatible_argument 137
mypy/build.py:write_cache 136
mypy/test/typefixture.py:__init__ 135
mypy/plugins/dataclasses.py:collect_attributes 128
mypy/typeshed/tests/pytype_test.py:pytype_test 126
mypy/server/update.py:update_module_isolated 121
mypy/build.py:validate_meta 121
mypy/test/data.py:parse_test_case 120
mypy/modulefinder.py:_find_module 116
mypy/server/update.py:reprocess_nodes 108
mypy/semanal_namedtuple.py:build_namedtuple_typeinfo 107
mypy/fastparse.py:do_func_def 104
mypy/semanal.py:_visit_func_def 103
mypy/semanal.py:visit_call_expr 102
mypy/checkexpr.py:check_overload_call 102
mypy/test/testcheck.py:run_case_once 102
mypy/constraints.py:visit_instance 98
mypy/checkexpr.py:check_op 97
mypy/semanal_pass1.py:visit_file 96
mypy/build.py:load_graph 96
mypy/build.py:__init__ 96
mypy/fastparse2.py:visit_FunctionDef 96
mypy/checker.py:check_override 93
mypy/build.py:find_module_and_diagnose 92
mypy/checker.py:check_assignment 91
mypy/messages.py:has_no_attr 91
mypy/exprtotype.py:expr_to_unanalyzed_type 91
mypy/checkexpr.py:check_argument_count 90
mypy/plugins/dataclasses.py:transform 89
mypy/checkmember.py:analyze_member_var_access 89
mypy/semanal.py:visit_member_expr 88
mypy/checker.py:check_overlapping_overloads 88
mypy/test/testfinegrained.py:run_case 88
mypy/messages.py:report_protocol_problems 86
mypy/server/deps.py:visit_assignment_stmt 84
mypy/checker.py:check_method_override_for_base_with_name 83
mypy/checkexpr.py:infer_overload_return_type 83
mypy/checkmember.py:analyze_class_attribute_access 83
mypy/checkexpr.py:visit_call_expr_inner 82
mypy/server/update.py:lookup_target 82
mypy/build.py:load_plugins 82
mypy/server/update.py:update 81
mypy/checkexpr.py:combine_function_signatures 80
Is this issue still relevant? Might pick this one up.
The mypy codebase has quite a few long functions, which is a problem since long functions are often hard to understand and modify. Once you can't see the whole function without scrolling, it's probably too long.
I think that we should refactor at least some of the longest functions into smaller ones. One way would be to systematically find all the longest functions and split them if they are hard to understand.
Not sure what is a good threshold. My editor can show 97 rows at a time on the display I typically use. I think that 97 rows is already too much most of the time. 75 rows per function might be a reasonable threshold above which we should consider taking action in code reviews. Thoughts?