Refactor function parameters to describe the behavior instead of the state which is interpreted in non-obvious ways

[MadLittleMods]

as a general rule, I would say it is preferable for method parameters to describe a change in behaviour ("inhibit_sync_to_clent") rather than describe some bit of state that the function is free to interpret as it wishes: it's much clearer for a later reader to understand how things tie together, particularly when things then get modified later on. Obviously this isn't a hard-and-fast rule, but it's worth considering.

[Currently we have a bit of a mess in some places (FederationEventHander.backfilled being a prime example) where you have to dig through 150 methods to find out what a flag actually does, and then somehow reverse-engineer what it is supposed to do. Don't do that.]

-- @richvdh, #11265 (comment)

Examples:

The interesting spots will be any of the bool function parameters, search for : bool.*=?.*,:

outlier and backfilled can be refactored out to options like:

• inhibit_sync_to_client
• inhibit_push_notifications
• use_negative_stream_ordering
• etc

is_peeking could be refactored into

• inhibit_peekers_viewing_shared_rooms
• include_membership
• etc

To refactor, it's probably easiest to dive down to the innermost function to find the actual functionality each function parameter triggers and pipe the new options to the top.

In many cases, we rely on event.internal_metadata with outlier (or is_outlier), historical (or is_historical), soft_failed (or is_soft_failed), etc. There are many side-effects from setting something as an outlier for example. We could write methods like event.internal_metadata.should_proactively_send() with these descriptive behaviors based on those properties, new ex. event.internal_metadata.should_inhibit_sync_to_client().

Definition of done

• Replace backfilled function parameters we pipe everywhere with specific behavior options
  • Related PR's:
    • Refactor backfilled into specific behavior function arguments (_persist_events_and_state_updates) #11417
• Replace outlier with specific behavior options
• Replace historical with specific behavior options (there will be some overlap with outlier and backfilled
• Replace is_peeking with specific behavior options

---

Keywords:

• Refactor logic to rely on descriptive behavior function parameters instead of interpreting a given field

[richvdh]

This issue strikes me as a bit open-ended: it essentially amounts to "make the codebase better", which is something that we can never, ever, call "done".

It might be ok if the issue cited specific things that need changing, but in general I'd say that it's not worth tracking refactoring tasks like that in an issue: if it bothers you enough, Just F-ing Do It (most likely as part of other work in the area); it's unlikely we'd be able to schedule an issue like this in its own right.

[MadLittleMods]

It's open ended but it still serves as a good reference for the rationale for us striving to get away from state-interpretations. I've added a definition of done to the description so we can close at some point.

I planned on making some of these changes for the examples given at least.

[MadLittleMods]

Closing as this has lived long enough. Still have aspirations to clean up historical and more backfilled to leave the codebase better than I found it