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

Find longest functions and split them #5917

Open
JukkaL opened this issue Nov 19, 2018 · 5 comments
Open

Find longest functions and split them #5917

JukkaL opened this issue Nov 19, 2018 · 5 comments
Labels
needs discussion refactoring Changing mypy's internals

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 19, 2018

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?

@ilevkivskyi ilevkivskyi added refactoring Changing mypy's internals needs discussion labels Nov 19, 2018
@ilevkivskyi
Copy link
Member

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.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 19, 2018

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.

@ilevkivskyi
Copy link
Member

OK, I agree.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 19, 2018

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

@williamsantosa
Copy link
Contributor

Is this issue still relevant? Might pick this one up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion refactoring Changing mypy's internals
Projects
None yet
Development

No branches or pull requests

3 participants