Improve Config definition API#926
Draft
matejmatuska wants to merge 4 commits into
Draft
Conversation
The Config class was defined using ABCMeta metaclass and properties were
defined as abstract properties. However, there are problems with that.
ABSTRACT PROPERTIES DO NOT ENFORCE CONSTRAINTS ON UNINSTANTIATED CLASSES:
Abstract properties only trigger a TypeError when a class is
instantiated. Because Config subclasses are accessed directly via the
class itself (e.g. SomeConfig.name) and never initialized, Python's
runtime does not enforce missing overrides.
Instead, accessing an un-overridden (technically just shadowed) property
on the class simply returns the property object itself, masking
potential bugs:
>>> import abc
>>> class A(metaclass=abc.ABCMeta):
... @abc.abstractproperty
... def a():
... pass
...
>>> A.a
<abc.abstractproperty object at 0x7fb816981550>
>>> class B(A):
... pass
...
>>> B.a # no error here, even though B doesn't override A <------------
<abc.abstractproperty object at 0x7fb816981550>
If there is a missing override there is no runtime error, just a linter
warning. There is nothing forcing the subclass to override the
abstractproperty at runtime, abstract class doesn't does not do it
because the class is never instantiated
TYPE-CHECKING INCOMPATIBILITIES:
Moreover, the subclasses of Config are supposed to "override" (shadow)
the abstract properties with class variables, but this is not a
correct override and type checking tools report it as error, e.g. for 'name':
class SomeConfig(Config):
section = 'some section'
name = "a_setting"
type_ = fields.String()
default = []
description = """
Some description
"""
These errors are reported (and similar for the other members):
1. "name" incorrectly overrides property of same name in class "Config" [reportIncompatibleMethodOverride]
2. Type "Literal['a_setting']" is not assignable to declared type "abstractproperty"
"Literal['source_clients']" is not assignable to "abstractproperty" [reportAssignmentType]
THE FIX:
This patch replaces the abstract properties with regular class
variables, as is done in other framework entities (see e.g. class Tag).
Though technically there is no overriding of class variables, just
shadowing in the subclass, it's not a problem as the class vars are
always accessed through the subclass, which was required also required
with the abstract properties.
There is also nothing forcing the subclass to override the class
variables, this will be addressed in a following commit. However the
types are consistent and don't trigger type checking errors.
They also have to default to something, for which I've chosen None,
which is arguably better than defaulting to type abc.abstractproperty.
matejmatuska
force-pushed
the
fix-config-definition
branch
from
819f5a1 to
e7ee17d
Compare
Member
Author
|
I improved the commit message for the first commit, hopefully it's easier to follow now, lmk if not. |
Add following checks for Config subclass definitions: - all class variables from Config are "overridden" (defined) in the subclass to a non null value - the type_ class var is set to an instance of a subclass of leapp.fields.Field - the default= in type_ is None (best approximation to not set) because this is ignored and the default class var is used This makes the API more robust and prevents invalid values at subclass definition time. Instead of a metaclass the __init_subclass__ method is used which is available in > python3.6.
Subclasses of Config act as schema definitions and should not be instantiated. Make this enforced in the API.
The function retrieve_config references self.config_schema, where self is Actor instance, config_schema doesn't exist in Actor, only config_schemas.
matejmatuska
force-pushed
the
fix-config-definition
branch
from
e7ee17d to
693b832
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found some annoyances when working with the Config API, the commit msgs should have all the info.
I think it should be essentially backwards compatible with the previous version + the added checks which when violated would either way result in an error elsewhere.
There is also a fix for AttributeError in retrieve_config.
TODO tests