Conversation
Barecheck - Code coverage reportTotal: 96.7%Your code coverage diff: 0.06% ▴ |
|
This now works and could be merged. Dealing with I think this is clear and consistent, and safer than defining global feature flags. However, it does mean that a descriptor defined on a base class could operate differently on two derived classes. The child classes don't mutate the base class, but they do have their own settings. We may want to consider checking I think this is a reasonable solution. I'm open to discussion of whether it's the right solution - I will try not to fall too far into being wedded to sunk costs... |
rwb27
force-pushed
the
remove-feature-flags
branch
from
b732e91 to
7a66580
Compare
rwb27
force-pushed
the
remove-feature-flags
branch
from
cdf8a73 to
832d161
Compare
This adds a class variable to give the Thing settings. This is overwritten by a descriptor in __init_subclass__ to make it read-only.
This does a few things to remove the need for FEATURE_FLAGS: * BaseDescriptor gains a property `owning_class` * Thing gains a class attribute `_class_settings` which is a `TypedDict` * The class settings typed dict is moved to its own module, and getter functions are defined to define default values etc. * I moved some exceptions out of `base_descriptor` and into `exceptions`. In the end I only needed them from `base_descriptor` but I've left them in `exceptions` as it would be good to have them centralised.
This fixes example code and adds a note that class settings are intended to be set only once, at definition time.
This filters warnings correctly and removes tests for FEATURE_FLAGS which is now removed.
Currently, this checks for `validate_properties_on_set` - as more keys are added, we should include them in testing. There are explicit tests in `test_thing_class_settings` as well as tests in the context of property set operations in `test_property`. I fixed a couple of minor things (error messages etc.) as a result of testing.
This uses and tests the `owning_class` property to deduplicate code and give helpful errors. It introduces a new exception specifically for the case where a class is garbage collected. This is unlikely, but now tested.
Rather than rely on `__init_subclass__` ensuring the settings dictionary exists (though it may still be empty), we now use a function that is robust to a missing dictionary. This should work much better with e.g. mixin classes where `__init_subclass__` will not get called.
Previously, I was using the settings on the class where properties were defined. This could lead to different behaviour between properties on the same object, if some properties are inherited from a base class or mixin.
I've swapped `type(obj)` for `obj.__class__` as it can be mocked. I've updated test code (mostly mocking) to avoid confusion.
Now that we read settings from the final class, it's less helpful to check the key is set on every base class. We now check the key is present when it's read. This may result in more warnings, but fewer spurious ones. Repeated warnings can be suppressed in the usual way.
rwb27
force-pushed
the
remove-feature-flags
branch
from
832d161 to
e778e0a
Compare
|
Not for this PR, but we may want in the future to have something in |
|
We thought about the name and we can't come up with a better one, so |
This now has a clearly specified default for `validate_properties_on_set` and links to the page on optional features for the rationale about how defaults might change. I've also fixed a few ambiguous cross-references while I'm here.
rwb27
force-pushed
the
remove-feature-flags
branch
from
84d640c to
28bbd0d
Compare
2 participants
This removes the global
FEATURE_FLAGSin favour of per-class settings. This should be much safer. We may in the future want a mechanism to check that the feature is enabled everywhere, but for things like property validation, I think per-class settings are fine.Things to do before merge:
Thing._class_settingsBaseDescriptor.owning_classBaseDescriptor.owning_classin other places to deduplicate codeOFM Feature Branch: v3-labthings-validate-properties-on-set