Require widget to implement compute_max_intrinsic about druid HOT 7 OPEN

I'm a bit conflicted here. I'm not sure it's reasonable to assume that compute_max_intrinsic will always be propagated recursively. The way druid allows custom implementation of its traits makes it hard to have firm rules like this. And going back to the original PR that introduced it, I'm pretty sure it was an explicit choice not to impose a new required method on all custom widgets.

That said, I'm 100% on board with adding the propagation to EnvScope and whatever other built-in widgets it makes sense for.

from druid.

@sjoshid @jneem since you authored and reviewed, respectively, the PR that introduced this method, would you have thoughts here?

from druid.

Hmmm. What can't you override the blanket impl? Like, many other widgets.

from druid.

Hmmm. What can't you override the blanket impl? Like, many other widgets.

You can override it, but because it's not required, the widget authors may forget to implement it properly (ie, call the compute_max_intrinsic on the child), making any container widget that relies on the default implementation break the call chain and result in weird, hard-to-debug behavior. Container widgets should always propagate the calculation to its child/children but atm this isn't required.

from druid.

All out-of-the-box containers handle it correctly. If it's a custom container, the onus is on the author of that container to override correctly. Druid, at the end of the day, is an opinioned framework.

Besides, if you remove the blanket impl, every widget out-of-the-box or not, must have an impl. What should be this default impl?

from druid.

Also, I havent used Druid in a long time. If @jneem thinks what you propose makes sense, I have no objection. :)

from druid.

All out-of-the-box containers handle it correctly. If it's a custom container, the onus is on the author of that container to override correctly. Druid, at the end of the day, is an opinioned framework.

Yeah, maybe it's just a matter of educating authors and fixing up the widgets in /widget that don't properly propagate the calculation. Any widget that contains a child widget or a child WidgetPod should call the compute_max_intrinsic. Taking a glance at /widget, it appears most don't properly call compute_max_instrinsic but instead rely on the blanket implementation. So they should be fixed up.

from druid.