Mitigating implicit chain breaking by "builder-style" methods embedded from *Box
This was mentioned a number of times in issues such as #141, #831, #916, etc., and it bit me in my back a number of times.
This behavior has been documented, however I still find it very error-prone, since I can never know when it's safe to use them. For example:
// Where does the chain downgrade to *tview.Box?..
confirmForm := tview.NewForm().SetButtonStyle(...).SetBackgroundColor(...).SetBorder(...)
// Let's make it move obvious:
confirmForm := tview.NewForm()
confirmForm.SetButtonStyle(...)
confirmForm.SetBackgroundColor(...)
confirmForm.SetBorder(...)
// Too verbose, let's join them back while keeping the declaration:
confirmForm := tview.NewForm()
confirmForm.SetButtonStyle(...).SetBackgroundColor(...).SetBorder(...)The last option seems to be the least error-prone but requires the user to be aware of this pattern.
IMHO we're mixing two concepts with those "builder" methods - constructing something vs configuring something - into a single self-returning "func (*X) foo() *X".
I think the only way to enforce this would be to have "builder" objects separate from "primitive" objects.
Here's my proposal. It's a backward-incompatible change, so I'm posting it here just for discussion and to entertain various ways of mitigating current behavior:
- Have
tview.NewXXX()return primitive*P(present behavior). - Make
*P.SetXXX()returns primitive's "downgraded builder object"*PB(which does NOT implement primitive). *Pembeds*PBso it gets all the setters/getters from builder for convenience.- Escaping from
*PBback to*Bmay be done with a certainBuild()method (escape hatch in case someone wants to have a one-line initialization).
This would make the following constraints possible to enforce:
// 1: legacy style (new + separate chain)
confirmForm := tview.NewForm() // *tview.Form
confirmForm.SetButtonStyle(...). // *tview.FormBuilder
SetBackgroundColor(...). // *tview.BoxBuilder
SetBorder(...) // *tview.BoxBuilder
// 2: preventing the issues referenced in the beginning of this post:
confirmForm := tview.NewForm().SetBorder(...)
flex.AddItem(confirmForm, ...) // error: tview.FormBuilder does not implement tview.Primitive
// 3: keeping the builder-style chain:
confirmForm := tview.NewForm().SetBorder(...).SetBackgroundColor(...).SetBorder(...).Build()
// confirmForm is now *tview.FormOpen issues:
tview.Box.Buildmust have to keep knowledge about the builder that invoked it.
Alternatives:
- tview could generalize the box so that the box can return its original container.
- tview could introduce "owner" or "top" property into Box and return it instead of its own instance (if set). However, it would require Box methods to start returning interfaces since the Box does not know its parent type.
Just wondering what y'all think. I'm not pushing this proposal, I'm just willing to talk about it and see if there are other ways of making this less error-prone.
Source: rivo/tview