WPB-21964: move feature flags logic to wire-subsystems #4941
Conversation
zebot
added
the
ok-to-test
battermann
left a comment
battermann
left a comment
There was a problem hiding this comment.
please add a changelog
There was a problem hiding this comment.
does this have to stay in galley? why?
There was a problem hiding this comment.
It depends on things only definable in galley, such as Opts.
There was a problem hiding this comment.
That alone is solvable by extracting a dedicated config for the subsystem. Are there still other reasons?
There was a problem hiding this comment.
It seems so, but it is the wrong tradeoff.
Pros:
- interpreters and effects are co-located (for one usage)
Cons:
- high coupling of
galleywith all packages depending onwire-subsystems - low cohesion, as all changes will impact all the dependant packages
Having separated effects definitions/interpreters, enforcing high cohesion and low coupling, is the purpose of effects systems.
| global <- inputs (view $ settings . checkGroupInfo) | ||
| guard (global == Just True) | ||
| mls <- getFeatureForTeam @MLSConfig tid | ||
| mls <- getFeatureForTeam @_ @MLSConfig tid |
There was a problem hiding this comment.
Can we change the order of the type parameters so that we do not have to pass a placeholder type?
There was a problem hiding this comment.
I'm not sure, it's polysemy defined
There was a problem hiding this comment.
Just tried with foralls at effect level, it does not work
| SConversationMemberUpdateTag -> do | ||
| void $ ensureOtherMember lconv (cmuTarget action) conv | ||
| E.setOtherMember lcnv (cmuTarget action) (cmuUpdate action) | ||
| let action' = action :: ConversationMemberUpdate |
There was a problem hiding this comment.
Why is this suddenly necessary?
There was a problem hiding this comment.
When making the change, GHC was unable to properly infer it, I do not know why (I welcome any idea), this is the workaround I have found.
|
@akshaymankar @battermann I have moved the interpreters in |
Checklist
changelog.d