refactor: Standardize nullability of the EnumProperty type parameter on event diagnostic messages [proposal]#181112
refactor: Standardize nullability of the EnumProperty type parameter on event diagnostic messages [proposal]#181112luanpotter wants to merge 1 commit into
Conversation
…on event diagnostic messages
|
I understand the reasoning, but I'm concerned that the API change will probably be breaking. In other words, existing apps that use a nullable type here will get compiler errors. Given how this change is mostly just to make the code pretty, I don't think the breakage can be justified, unfortunately. Can we still fix the usages without changing the API? Edit: On another thought, I think being able to support nullable enums is important. How else would you like a class to describe its nullable enum property without this feature? Wrapping the |
|
Yeah I wasn't super happy with it in the end. I think my main disagreement with the current structure is that if we want the |
On the
debugFillPropertieschain used to print debug representation of events, theEnumPropertyextends the cornerstoneDiagnosticsPropertywith a typeT. The typeTis not meant to be nullable because the values are always supportive of nulls regardless of the value ofT(i.e.valueis of typeT?). This can be seen on the vast majority of cases and children ofDiagnosticsPropertywhich do not specifyFoo?.However
EnumPropertywas defined to support a nullableT, which cause some usages (but not most) to specify the null version of the enum (such asEnumProperty<PointerDeviceKind?>), which was not consistent even with many other usages. This makes it identical to the field definition, which can benulland thus is defined asPointerDeviceKind?, but it is ultimately not necessary with howDiagnosticsPropertydefines it.This PR removes the permissibility of nullable
TonEnumProperty, fixing the few existing violations which makes them standardized with all other usages ofEnumProperty(andDiagnosticsPropertyin general).This should have absolutely no change in behaviour whatsoever, and this "debug to string" property is covered by the
debug_test.dartand should be unchanged.This was extracted from #176968 in an attempt to simplify it.
Note: this does not fix all other children or direct usages of
DiagnosticsProperty, which there are a few. Those can be done as followups if desired.Note: if the intent is to have the type of
Tto match the type of the field exactly (including nullability), I think it would make sense to then change the type ofvalueto be justT- that way it "forces" the user to specifyFoo?if the type is nullableFoo?. That is a good alternative as well, that if preferred, I can pivot to.Pre-launch Checklist
///).