ref(alerts): Refactor DetailedWorkflowEngineDetectorSerializer to remove inheritance#110706
ref(alerts): Refactor DetailedWorkflowEngineDetectorSerializer to remove inheritance#110706
Conversation
| base_data = self.base_serializer.serialize(obj, attrs, user, **kwargs) | ||
| data: DetailedAlertRuleSerializerResponse = { |
There was a problem hiding this comment.
really like how this reads in code!
any thoughts on ways we might be able to support other folks in knowing the best path forward for these? (iirc the hope for using inheritance was to reduce the number of touch points required for consumers of the platform to modify).
Should the base class follow a registry pattern? should we create flat helper methods? should just create an abc that says "add the base_validator" property like this? 🤔 lots of ways to approach it; i think the inheritance pattern was the established one, but i'm not a big fan of "lasagna" code.
There was a problem hiding this comment.
I don't have any great ideas, but I think:
- Treat
castand cycle-breaking import as structural bugs. Not all solvable, but all worth solving. - Treat implementation inheritance for code-sharing with extreme suspicion; can be fine, but in the claude era we don't need to feel too bad about well-specified boilerplate translation layers.
But this particular case was my fault, so I'm happy to be able to fix it.
For serializers and validators in general, I think the support for hierarchical composition for reuse is actually really strong. The case for inheritance based code sharing (particularly with validators) is much weaker. Absent a graphql sort of world, we may want to be leaning more toward structures that allow extending to be a explicit composition (eg one key for X fields, one key for additional data, rather than merging them into one object, even though that's possible). I haven't thought enough about it to have a worthwhile opinion, though.
That said, I'm currently very in favor of avoiding ever using partial=True for Validators. Having a mode where required stops meaning anything is convenient in some cases, but makes the code really hard to make reliable.
Implemenetation inheritance is always a little iffy, and this one (at least according to mypy) was a LSP violation.
Better to just fix it than to try to cast it away.