Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add flake8 --max-complexity checking #9367

Closed
ShadowJonathan opened this issue Feb 10, 2021 · 2 comments
Closed

Add flake8 --max-complexity checking #9367

ShadowJonathan opened this issue Feb 10, 2021 · 2 comments
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@ShadowJonathan
Copy link
Contributor

This maybe a bit unhelpful as a check, but in flake8, while researching #9366, i encountered that max-complexity for all C90 violations were never checked.

When i enable this option, i get the following;

+ flake8 synapse docker tests scripts-dev scripts contrib synctl setup.py synmark stubs .buildkite --max-complexity 10
synapse/visibility.py:49:7: C901 'filter_events_for_client' is too complex (31)
synapse/visibility.py:265:7: C901 'filter_events_for_server' is too complex (29)
synapse/notifier.py:429:11: C901 'Notifier.wait_for_events' is too complex (11)
synapse/event_auth.py:38:1: C901 'check' is too complex (26)
synapse/event_auth.py:222:1: C901 '_is_membership_change_allowed' is too complex (29)
synapse/event_auth.py:461:1: C901 '_check_power_levels' is too complex (20)
synapse/event_auth.py:608:1: C901 '_verify_third_party_invite' is too complex (16)
synapse/python_dependencies.py:150:1: C901 'check_requirements' is too complex (14)
synapse/storage/database.py:493:5: C901 'DatabasePool.new_transaction' is too complex (14)
synapse/storage/persist_events.py:330:11: C901 'EventsPersistenceStorage._persist_events' is too complex (19)
synapse/storage/persist_events.py:602:11: C901 'EventsPersistenceStorage._get_new_state_after_events' is too complex (20)
synapse/storage/persist_events.py:926:11: C901 'EventsPersistenceStorage._is_server_still_joined' is too complex (12)
synapse/storage/prepare_database.py:275:1: C901 '_upgrade_existing_database' is too complex (26)
synapse/storage/databases/state/bg_updates.py:199:11: C901 'StateBackgroundUpdateStore._background_deduplicate_state' is too complex (11)
synapse/storage/databases/main/user_directory.py:139:11: C901 'UserDirectoryBackgroundUpdateStore._populate_user_directory_process_rooms' is too complex (20)
synapse/storage/databases/main/events.py:476:5: C901 'PersistEventsStore._add_chain_cover_index' is too complex (17)
synapse/storage/databases/main/events.py:755:5: C901 'PersistEventsStore._allocate_chain_ids' is too complex (11)
synapse/storage/databases/main/events.py:1398:5: C901 'PersistEventsStore._update_metadata_tables_txn' is too complex (13)
synapse/storage/databases/main/event_federation.py:183:5: C901 'EventFederationWorkerStore._get_auth_chain_difference_using_cover_index_txn' is too complex (17)
synapse/storage/databases/main/event_federation.py:341:5: C901 'EventFederationWorkerStore._get_auth_chain_difference_txn' is too complex (14)
synapse/storage/databases/main/search.py:118:11: C901 'SearchBackgroundUpdateStore._background_reindex_search' is too complex (13)
synapse/storage/databases/main/search.py:462:11: C901 'SearchStore.search_rooms' is too complex (11)
synapse/storage/databases/main/devices.py:130:5: C901 'DeviceWorkerStore.get_device_updates_by_remote' is too complex (11)
synapse/storage/databases/main/room.py:207:11: C901 'RoomWorkerStore.get_largest_public_rooms' is too complex (11)
synapse/storage/databases/main/room.py:359:11: C901 'RoomWorkerStore.get_rooms_paginate' is too complex (21)
synapse/storage/databases/main/stats.py:751:11: C901 'StatsStore._calculate_and_set_initial_state_for_room' is too complex (11)
synapse/storage/databases/main/events_bg_updates.py:278:11: C901 'EventsBackgroundUpdatesStore._cleanup_extremities_bg_update' is too complex (15)
synapse/storage/databases/main/events_bg_updates.py:613:11: C901 'EventsBackgroundUpdatesStore._rejected_events_metadata' is too complex (13)
synapse/storage/databases/main/roommember.py:536:5: C901 'RoomMemberWorkerStore._get_joined_users_from_context' is too complex (13)
synapse/storage/databases/main/stream.py:1033:5: C901 'StreamWorkerStore._paginate_room_events_txn' is too complex (12)
synapse/storage/databases/main/group_server.py:636:5: C901 'GroupServerStore._add_room_to_summary_txn' is too complex (11)
synapse/storage/databases/main/group_server.py:881:5: C901 'GroupServerStore._add_user_to_summary_txn' is too complex (11)
synapse/storage/databases/main/events_worker.py:301:11: C901 'EventsWorkerStore.get_events_as_list' is too complex (18)
synapse/storage/databases/main/events_worker.py:648:11: C901 'EventsWorkerStore._get_events_from_db' is too complex (20)
synapse/api/auth.py:296:11: C901 'Auth.get_user_by_access_token' is too complex (11)
synapse/api/auth_blocking.py:41:11: C901 'AuthBlocking.check_auth_blocking' is too complex (18)
synapse/appservice/__init__.py:88:5: C901 'ApplicationService._check_namespaces' is too complex (14)
synapse/state/v2.py:493:7: C901 '_iterative_auth_checks' is too complex (11)
...

A few of these functions indeed go on for several hundreds(!) of lines, with sprawling nesting and such.

While I know that this is a good example of "don't fix what ain't broke", i think it'll be less of a mental cognition burden if some of these warnings were at least taken seriously.

@erikjohnston
Copy link
Member

Complexity in general is one of those things that is really hard to measure. A lot of the time splitting up long functions is the right thing to do, but equally sometimes it makes it more complex. Note e.g. get_rooms_paginate is considered complex because it has a long set of if/else statements in it, and very little else. While you could move that set of if statements out to a separate function I bet it would complain about that.

(This is just a long winded way of saying complexity measures in most forms tend to always have false positives)

@erikjohnston erikjohnston added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Feb 11, 2021
@clokep
Copy link
Member

clokep commented Jun 8, 2023

We no longer use flake8, ruff likely has this option also but as @erikjohnston said I don't think we want this.

@clokep clokep closed this as completed Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

3 participants