Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling help executes @classmethod @property decorated methods #89519

Closed
randolf-scholz mannequin opened this issue Oct 3, 2021 · 40 comments
Closed

Calling help executes @classmethod @property decorated methods #89519

randolf-scholz mannequin opened this issue Oct 3, 2021 · 40 comments
Assignees
Labels
3.11 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@randolf-scholz
Copy link
Mannequin

randolf-scholz mannequin commented Oct 3, 2021

BPO 45356
Nosy @rhettinger, @terryjreedy, @ambv, @berkerpeksag, @serhiy-storchaka, @wimglenn, @corona10, @pablogsal, @akulakov, @sirosen, @randolf-scholz, @AlexWaygood
PRs
  • bpo-45356: fix incorrect access of class property in pydoc and inspect #29239
  • Files
  • classmethod_property.py
  • classmethod_property.py
  • ClassPropertyIdea.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/rhettinger'
    closed_at = None
    created_at = <Date 2021-10-03.19:45:55.963>
    labels = ['type-bug', '3.11']
    title = 'Calling `help` executes @classmethod @property decorated methods'
    updated_at = <Date 2022-02-17.20:03:40.349>
    user = 'https://github.com/randolf-scholz'

    bugs.python.org fields:

    activity = <Date 2022-02-17.20:03:40.349>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2021-10-03.19:45:55.963>
    creator = 'randolf.scholz'
    dependencies = []
    files = ['50325', '50326', '50344']
    hgrepos = []
    issue_num = 45356
    keywords = ['patch']
    message_count = 23.0
    messages = ['403109', '403110', '403111', '403504', '403505', '403578', '403582', '403611', '403613', '403653', '403699', '403713', '405100', '405115', '405121', '405142', '405143', '406600', '406605', '406606', '407493', '407499', '413451']
    nosy_count = 13.0
    nosy_names = ['rhettinger', 'terry.reedy', 'grahamd', 'lukasz.langa', 'berker.peksag', 'serhiy.storchaka', 'wim.glenn', 'corona10', 'pablogsal', 'andrei.avk', 'sirosen0', 'randolf.scholz', 'AlexWaygood']
    pr_nums = ['29239']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45356'
    versions = ['Python 3.11']

    Linked PRs

    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 3, 2021

    I noticed some strange behaviour when calling help on a class inheriting from a class or having itself @classmethod @Property decorated methods.

    from time import sleep
    from abc import ABC, ABCMeta, abstractmethod
    
    class MyMetaClass(ABCMeta):
        @classmethod
        @property
        def expensive_metaclass_property(cls):
            """This may take a while to compute!"""
            print("computing metaclass property"); sleep(3)
            return "Phew, that was a lot of work!"
    
        
    class MyBaseClass(ABC, metaclass=MyMetaClass):
        @classmethod
        @property
        def expensive_class_property(cls):
            """This may take a while to compute!"""
            print("computing class property .."); sleep(3)
            return "Phew, that was a lot of work!"
        
        @property
        def expensive_instance_property(self):
            """This may take a while to compute!"""
            print("computing instance property ..."); sleep(3)
            return "Phew, that was a lot of work!"
    
    class MyClass(MyBaseClass):
        """Some subclass of MyBaseClass"""
        
    help(MyClass)

    Calling help(MyClass) will cause expensive_class_property to be executed 4 times (!)

    The other two properties, expensive_instance_property and expensive_metaclass_property are not executed.

    Secondly, only expensive_instance_property is listed as a read-only property; expensive_class_property is listed as a classmethod and expensive_metaclass_property is unlisted.

    The problem is also present in '3.10.0rc2 (default, Sep 28 2021, 17:57:14) [GCC 10.2.1 20210110]'

    Stack Overflow thread: https://stackoverflow.com/questions/69426309

    @randolf-scholz randolf-scholz mannequin added 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error labels Oct 3, 2021
    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 3, 2021

    I updated the script with dome more info. The class-property gets actually executed 5 times when calling help(MyClass)

    Computing class property of <class '__main__.MyClass'> ...DONE!
    Computing class property of <class '__main__.MyClass'> ...DONE!
    Computing class property of <class '__main__.MyClass'> ...DONE!
    Computing class property of <class '__main__.MyBaseClass'> ...DONE!
    Computing class property of <class '__main__.MyClass'> ...DONE!
    

    @AlexWaygood
    Copy link
    Member

    See also: https://bugs.python.org/issue44904

    @terryjreedy
    Copy link
    Member

    Randolf, what specific behaviors do you consider to be bugs that should be fixed. What would a test of the the changed behavior look like?

    This should perhaps be closed as a duplicate of bpo-44904. Randolf, please check and say what you thing.

    @terryjreedy
    Copy link
    Member

    On current 3.9, 3.10, 3.11, on Windows running in IDLE, I see
    computing class property ..
    computing class property ..
    computing class property ..
    computing class property ..
    computing class property ..
    Help ...

    @terryjreedy terryjreedy added 3.11 only security fixes and removed 3.9 only security fixes 3.10 only security fixes labels Oct 8, 2021
    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 10, 2021

    @terry I think the problem boils down to the fact that @classmethod @property decorated methods end up not being real properties.

    Calling MyBaseClass.__dict__ reveals:

    mappingproxy({'__module__': '__main__',
                  'expensive_class_property': <classmethod at 0x7f893e95dd60>,
                  'expensive_instance_property': <property at 0x7f893e8a5860>,
                  '__dict__': <attribute '__dict__' of 'MyBaseClass' objects>,
                  '__weakref__': <attribute '__weakref__' of 'MyBaseClass' objects>,
                  '__doc__': None,
                  '__abstractmethods__': frozenset(),
                  '_abc_impl': <_abc._abc_data at 0x7f893fb98740>})

    Two main issues:

    1. Any analytics or documentation tool that has special treatment for properties may not identify 'expensive_class_property' as a property if they simply check via isinstance(func, property). Of course, this could be fixed by the tool-makers by doing a conditional check:
    isinstance(func, property) or (`isinstance(func, classmethod)` and `isinstance(func.__func__, property)`
    1. expensive_class_property does not implement getter, setter, deleter. This seems to be the real dealbreaker, for example, if we do
    MyBaseClass.expensive_class_property = 2
    MyBaseClass().expensive_instance_property = 2

    Then the first line erroneously executes, such that MyBaseClass.dict['expensive_class_property'] is now int instead of classmethod, while the second line correctly raises AttributeError: can't set attribute since the setter method is not implemented.

    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 10, 2021

    If fact, in the current state it seem that it is impossible to implement real class-properties, for a simple reason:

    descriptor.__set__ is only called when setting the attribute of an instance, but not of a class!!

    import math
    
    class TrigConst: 
        const = math.pi
        def __get__(self, obj, objtype=None):
            print("__get__ called")
            return self.const
        
        def __set__(self, obj, value):
            print("__set__ called")
            self.const = value
            
    
    class Trig:
        const = TrigConst()              # Descriptor instance
    Trig().const             # calls TrigConst.__get__
    Trig().const = math.tau  # calls TrigConst.__set__
    Trig.const               # calls TrigConst.__get__
    Trig.const = math.pi     # overwrites TrigConst attribute with float.

    @rhettinger
    Copy link
    Contributor

    I'm merging bpo-44904 into this one because they are related.

    @rhettinger
    Copy link
    Contributor

    It may have been a mistake to allow this kind of decorator chaining.

    • As Randolf and Alex have noticed, it invalidates assumptions made elsewhere in the standard library and presumably in third-party code as well.

    • And as Randolf noted in his last post, the current descriptor logic doesn't make it possible to implement classmethod properties with setter logic.

    • In bpo-44973, we found that staticmethod and property also don't compose well, nor does abstract method.

    We either need to undo the Python 3.9 feature from bpo-19072, or we need to put serious thought into making all these descriptors composable in a way that would match people's expectations.

    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 11, 2021

    Dear Raymond,

    I think decorator chaining is a very useful concept. At the very least, if all decorators are functional (following the functools.wraps recipe), things work out great -- we even get associativity of function composition if things are done properly!

    The question is: can we get similar behaviour when allowing decoration with stateful objects, i.e. classes? This seems a lot more difficult. At the very least, in the case of @classmethod I think one can formulate a straightforward desiderata:

    ### Expectation

    • classmethod(property) should still be a property!
    • More generally: classmethod(decorated) should always be a subclass of decorated!

    Using your pure-python versions of property / classmethod from https://docs.python.org/3.9/howto/descriptor.html, I was able to write down a variant of classmethod that works mostly as expected in conjunction with property. The main idea is rewrite the classmethod to dynamically be a subclass of whatever it wrapped; roughly:

    def ClassMethod(func):
      class Wrapped(type(func)):
          def __get__(self, obj, cls=None):
              if cls is None:
                  cls = type(obj)
              if hasattr(type(self.__func__), '__get__'):
                  return self.__func__.__get__(cls)
              return MethodType(self.__func__, cls)
      return Wrapped(func)

    I attached a full MWE. Unfortunately, this doesn't fix the help bug, though it is kind of weird because the decorated class-property now correctly shows up under the "Readonly properties" section. Maybe help internally checks isinstance(cls.func, property) at some point instead of isinstance(cls.__dict__["func"], property)?

    ### Some further Proposals / Ideas

    1. Decorators could always have an attribute that points to whatever object they wrapped. For the sake of argument, let's take __func__.
      ⟹ raise Error when typing @decorator if not hasattr(decorated, "__func__")
      ⟹ Regular functions/methods should probably by default have __func__ as a pointer to themselves?
      ⟹ This could hae several subsidiary benefits, for example, currently, how would you implement a pair of decorators @print_args_and_kwargs and @time_execution such that both of them only apply to the base function, no matter the order in which they are decorating it? The proposed __func__ convention would make this very easy, almost trivial.
    2. type.__setattr__ could support checking if attr already exists and has __set__ implemented.
      ⟹ This would allow true class-properties with getter, setter and deleter. I provide a MWE here: https://mail.google.com/mail/u/0/#label/Python+Ideas/FMfcgzGlkPRbJVRkHHtkRPhMCxNsFHpl
    3. I think an argument can be made that it would be really, really cool if @ could become a general purpose function composition operator?
      ⟹ This is already kind of what it is doing with decorators
      ⟹ This is already exacltly what it is doing in numpy -- matrix multiplication *is* the composition of linear functions.
      ⟹ In fact this is a frequently requested feature on python-ideas!
      ⟹ But here is probably the wrong place to discuss this.

    @AlexWaygood
    Copy link
    Member

    Some thoughts from me, as an unqualified but interested party:

    Like Randolph, I very much like having class properties in the language, and have used them in several projects since their introduction in 3.9. I find they're especially useful with Enums. However, while the bug in doctest, for example, is relatively trivial to fix (see my PR in bpo-44904), it seems to me plausible that bugs might crop up in other standard library modules as well. As such, leaving class properties in the language might mean that several more bugfixes relating to this feature would have to be made in the future. So, I can see the argument for removing them.

    It seems to me that Randolph's idea of changing classmethod from a class into a function would break a lot of existing code. As an alternative, one small adjustment that could be made would be to special-case isinstance() when it comes to class properties. In pure Python, you could achieve this like this:

    oldproperty = property
    
    class propertymeta(type):
        def __instancecheck__(cls, obj):
            return super().__instancecheck__(obj) or (
                isinstance(obj, classmethod)
                and super().__instancecheck__(obj.__func__)
                )
    
    
    class property(oldproperty, metaclass=propertymeta): pass
    

    This would at least mean that isinstance(classmethod(property(lambda cls: 42)), property) and isinstance(classmethod(property(lambda cls: 42)), classmethod) would both evaluate to True. This would be a bit of a lie (an instance of classmethod isn't an instance of property), but would at least warn the caller that the object they were dealing with had some propertylike qualities to it.

    Note that this change wouldn't fix the bugs in abc and doctest, nor does it fix the issue with class-property setter logic that Randolph identified.

    With regards to the point that @staticmethod cannot be stacked on top of @property: it strikes me that this feature isn't really needed. You can achieve the same effect just by stacking @classmethod on top of @property and not using the cls parameter.

    @randolf-scholz
    Copy link
    Mannequin Author

    randolf-scholz mannequin commented Oct 12, 2021

    @alex Regarding my proposal, the crucial part is the desiderata, not the hacked up implementation I presented.

    And I really believe that at least that part I got hopefully right. After all, what does @classmethod functionally do? It makes the first argument of the function receive the class type instead of the object instance and makes it possible to call it directly from the class without instantiating it. I would still expect MyClass.__dict__["themethod"] to behave, as an object, much the same, regardless if it was decorated with @classmethod or not.

    Regarding code breakage - yes that is a problem, but code is already broken and imo Python needs to make a big decision going forward:

    1. Embrace decorator chaining and try hard to make it work universally (which afaik was never intended originally when decorators were first introduced). As a mathematician I would love this, also adding @ as a general purpose function composition operator would add quite some useful functional programming aspects to python.
    2. Revert changes from 3.9 and generally discourage decorator chaining.

    At this point however I want to emphasize that I am neither a CS major nor a python expert (I only started working with the language 3 years ago), so take everything I say as what it is - the opinion of some random stranger from the internet (:

    @akulakov
    Copy link
    Contributor

    I've put up a PR; I'm not sure it's the best way to fix it. I will look more into it and will try to post some details about the PR later today.

    @akulakov
    Copy link
    Contributor

    I missed that this is assigned to Raymond, hope we didn't duplicate any effort (it only took me a short while to do the PR). Apologies..

    @akulakov
    Copy link
    Contributor

    I've looked more into it, the issue is that even before an object can be tested with isinstance(), both inspect.classify_class_attrs and in pydoc, classdoc local function spill() use a getattr() call, which triggers the property.

    So I think my PR is going in the right direction, adding guards in the inspect function and in two spill() functions. If isinstance() can be fixed to detect class properties correctly, these guards can be simplified but they still need to be there, I think.

    @wimglenn
    Copy link
    Mannequin

    wimglenn mannequin commented Oct 28, 2021

    added Graham Dumpleton to nosy list in case he has some useful insight here, I think the PR from bpo-19072 may have been adapted from grahamd's patch originally?

    @grahamd
    Copy link
    Mannequin

    grahamd mannequin commented Oct 28, 2021

    Too much to grok right now.

    There is already a convention for what a decorator wraps. It is __wrapped__.

    wrapper.__wrapped__ = wrapped

    Don't use __func__ as that has other defined meaning in Python related to bound methods and possibly other things as well and overloading on that will break other stuff.

    In part I suspect a lot of the problems here are because things like classmethod and functools style decorators are not proper transparent object proxies, which is the point of what the wrapt package was trying to solve so that accessing stuff on the wrapper behaves as much as possible as if it was done on what was wrapped, including things like isinstance checks.

    @rhettinger
    Copy link
    Contributor

    Also see: https://bugs.python.org/issue42073

    The classmethod pass through broke some existing code and the "fix" for it looks dubious:

                if hasattr(type(self.f), '__get__'):
                    return self.f.__get__(cls, cls)

    @rhettinger
    Copy link
    Contributor

    I propose deprecating classmethod chaining. It has become clear that it doesn't really do what people wanted and can't easily be made to work.

    By even suggesting that some stateful decorators are composable, we've ventured onto thin ice. Wrapping property in a classmethod doesn't produce something that behaves like a real property. Mixing staticmethod and property doesn't work at all. Putting abstractmethod in the mix doesn't work well either. The ecosystem of code inspection tools, like help() in this issue, is wholly unprepared for recognizing and working around these combinations. The latest "fix" for classmethod chaining looks weird and worriesome as well: self.f.__get__(cls, cls).

    Classmethod chaining is relatively new, so we will affect very little code by deprecating it. Any of the possible use cases can be served in other ways like the wrapt package or by explicit code in __getattribute__.

    @AlexWaygood
    Copy link
    Member

    It makes me sad that the stdlib will no longer provide a way to compose classmethods with other descriptors. However, I agree that deprecating classmethod chaining is probably the correct course of action, given the complications this feature has caused, and the backwards-compatibility issues it raises.

    This is probably a conversation for another BPO issue or the python-ideas mailing list, but I hope some consideration can be given in the future as to whether a new classmethod-like feature could possibly be added to functools that would enable this kind of decorator chaining without the same code-breakage concerns that this feature has had.

    @sirosen
    Copy link
    Mannequin

    sirosen mannequin commented Dec 1, 2021

    Probably >90% of the use-cases for chaining classmethod are a read-only class property.
    It's important enough that some tools (e.g. sphinx) even have special-cased support for classmethod(property(...)).

    Perhaps the general case of classmethod(descriptor(...)) should be treated separately from the common case?

    I propose deprecating classmethod chaining. It has become clear that it doesn't really do what people wanted and can't easily be made to work.

    If classmethod(property(f)) is going to be removed, can an implementation of classproperty be considered as a replacement?

    Or perhaps support via the other ordering, property(classmethod(f))?

    @SimpleArt
    Copy link

    Concerning the chaining of @property and @classmethod, I did notice that there is potential to have both in a way which does not cause significant confusion by introducing a cget value which should roughly look like this:

    class property:
    
        def __init__(self, fget=None, fset=None, fdel=None, doc=None, cget=None):
            if doc is None and fget is not None:
                doc = fget.__doc__
            if isinstance(fget, classmethod):
                if cget is not None:
                    raise TypeError("got class getter with classmethod fget")
                cget = fget
                fget = None
            self.fget = fget
            self.fset = fset
            self.fdel = fdel
            self.__doc__ = doc
            self.cget = cget
    
        def __get__(self, instance, owner=None):
            if instance is None:
                return self.cget(owner)
            elif self.cget is None:  # The default behavior is to return the property object.
                return self
            else:
                return self.fget(owner)
    
        def getter(self, fget):
            if isinstance(fget, classmethod):
                return property(self.fget, self.fset, self.fdel, self.__doc__, fget)
            else:
                return property(fget, self.fset, self.fdel, self.__doc__, self.cget)

    Example usage:

    class C:
    
        @property
        @classmethod
        def is_instance(cls) -> bool:
            return False
    
        @is_instance.getter
        def is_instance(self) -> bool:
            return True
    
    
    print(C.is_instance)    # False
    print(C().is_instance)  # True

    Although I can't imagine a case where you'd want to use both, this would definitely let you consistently use each approach individually. The drawback that I see from this however is the fact that people might get confused and think that you can use @property.setter with @classmethod. An error could get raised at this point, but people might try to remove the @classmethod to avoid the error. I think that linters should then complain that the setter should take self instead of cls, really cementing the fact that a writable class property shouldn't work. Take it as you will.

    @sobolevn
    Copy link
    Member

    Now 3.13 is in the works, so this can be changed according to the TODO item in 3.11.rst:

    Pending Removal in Python 3.13
    ------------------------------
    
    * :class:`classmethod` descriptor chaining (:gh:`89519`)
    

    @ProgramRipper
    Copy link

    I notice that this feature will be removed by #110163 in Python 3.13. So are there any workaround? If so, that may be great if it could be added to the Descriptor HowTo Guide, because it has been commonly used.

    @Prometheus3375
    Copy link
    Contributor

    Here is the sample of the implementation. You can also use a metaclass to add a property.

    facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Oct 25, 2023
    …aining
    
    Summary:
    upstream issue [GH-89519](python/cpython#89519) is about removing classmethod descriptor chaining capability, which was added in 3.9, deprecated in 3.11, and pending removal in 3.13.
    
    we have code that breaks due to the "new" (to us, when upgrading from 3.8 ) descriptor chaining behavior, so instead of figuring out how to fix it, considering this is deprecated and being removed in upstream, it seems preferable to "accelerate" that removal internally by backporting the deprecation and removal in cinder 3.10.
    
    the stacked two diffs backport the deprecation (D50534764) and removal (D50534886). this diff deletes a couple of tests in cinder that rely on that behavior.
    
    Reviewed By: swtaarrs
    
    Differential Revision: D50578116
    
    fbshipit-source-id: 2faed200a3a130ff7be7d8c6a231c6431789b330
    facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Oct 25, 2023
    Summary: backporting upstream commit `ebaf0945f9f` (merged upstream [PR GH-92379](python/cpython#92379)), part of upstream issue [GH-89519](python/cpython#89519)
    
    Reviewed By: swtaarrs
    
    Differential Revision: D50534764
    
    fbshipit-source-id: e74c8910e56f117b3c1cab3442de7394b0bd8b22
    facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Oct 25, 2023
    …11 (#110163)
    
    Summary: backporting [Raymond Hettinger's `remove_classmethod_descriptor_chaining` branch](https://github.com/rhettinger/cpython/tree/remove_classmethod_descriptor_chaining) (pending upstream PR [GH-110163](python/cpython#110163)), part of upstream issue [GH-89519](python/cpython#89519)
    
    Reviewed By: swtaarrs
    
    Differential Revision: D50534886
    
    fbshipit-source-id: 05b29193ce220f0d91fd0df8d5cef34d7e923456
    rhettinger added a commit to rhettinger/cpython that referenced this issue Dec 17, 2023
    rhettinger added a commit that referenced this issue Dec 21, 2023
    Restore behaviors before classmethod descriptor chaining was introduced.
    facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Dec 22, 2023
    …ted since 3.11)
    
    Summary:
    backport upstream PRs python/cpython#110163 and python/cpython#113233
    
    upstream issues: python/cpython#89519 (Calling help executes classmethod property decorated methods) and python/cpython#113157 (Changed behavior of <instancemethod>.__get__ in Python 3.11)
    
    upstream commits: [`7f9a99e8549b792662f2cd28bf38a4d4625bd402`](python/cpython@7f9a99e) and [`d058eaeed44766a8291013b275ad22f153935d3b`](python/cpython@d058eae)
    
    Reviewed By: aleivag
    
    Differential Revision: D52014322
    
    fbshipit-source-id: 87de6d9587bd9cc49f053ca340adfc469b041f91
    ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
    Restore behaviors before classmethod descriptor chaining was introduced.
    kulikjak pushed a commit to kulikjak/cpython that referenced this issue Jan 22, 2024
    Restore behaviors before classmethod descriptor chaining was introduced.
    aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
    aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
    Restore behaviors before classmethod descriptor chaining was introduced.
    QMalcolm added a commit to dbt-labs/dbt-core that referenced this issue Feb 13, 2024
    In the next commit we're gonna add a `to_resource` method. As we don't
    want to have to pass a resource into `to_resource`, the class itself
    needs to expose what resource class should be built. Thus a type annotation
    is no longer enough. To solve this we've added a class method to BaseNode
    which returns the associated resource class. The method on BaseNode will
    raise a NotImplementedError unless the the inheriting class has overridden
    the `resouce_class` method to return the a resource class.
    
    You may be thinking "Why not a class property"? And that is absolutely a
    valid question. We used to be able to chain `@classmethod` with
    `@property` to create a class property. However, this was deprecated in
    python 3.11 and removed in 3.13 (details on why this happened can be found
    [here](python/cpython#89519)). There is an
    [alternate way to setup a class property](python/cpython#89519 (comment)),
    however this seems a bit convoluted if a class method easily gets the job
    done. The draw back is that we must do `.resource_class()` instead of
    `.resource_class` and on classes implementing `BaseNode` we have to
    override it with a method instead of a property specification.
    
    Additionally, making it a class _instance_ property won't work because
    we don't want to require an _instance_ of the class to get the
    `resource_class` as we might not have an instance at our dispossal.
    QMalcolm added a commit to dbt-labs/dbt-core that referenced this issue Feb 15, 2024
    * Move `ColumnInfo` to dbt/artifacts
    
    * Move `Quoting` resource to dbt/artifacts
    
    * Move `TimePeriod` to `types.py` in dbt/artifacts
    
    * Move `Time` class to `components`
    
    We need to move the data parts of the `Time` definition to dbt/artifacts.
    That is not what we're doing in this commit. In this commit we're simply
    moving the functional `Time` definition upstream of `unparsed` and `nodes`.
    This does two things
      - Mirrors the import path that the resource `time` definition will have in dbt/artifacts
      - Reduces the chance of ciricular import problems between `unparsed` and `nodes`
    
    * Move data part of `Time` definition to dbt/artifacts
    
    * Move `FreshnessThreshold` class to components module
    
    We need to move the data parts of the `FreshnessThreshold` definition to dbt/artifacts.
    That is not what we're doing in this commit. In this commit we're simply
    moving the functional `FreshnessThreshold` definition upstream of `unparsed` and `nodes`.
    This does two things
      - Mirrors the import path that the resource `FreshnessThreshold` definition will have in dbt/artifacts
      - Reduces the chance of ciricular import problems between `unparsed` and `nodes`
    
    * Move data part of `FreshnessThreshold` to dbt/artifacts
    
    Note: We had to override some of the attrs of the `FreshnessThreshold`
    resource because the resource version only has access to the resource
    version of `Time`. The overrides in the functional definition of
    `FreshnessThreshold` make it so the attrs use the functional version
    of `Time`.
    
    * Move `ExternalTable` and `ExternalPartition` to `source_definition` module in dbt/artifacts
    
    * Move `SourceConfig` to `source_definition` module in dbt/artifacts
    
    * Move `HasRelationMetadata` to core `components` module
    
    This is a precursor to splitting `HasRelationMetadata` into it's
    data and functional parts.
    
    * Move data portion of `HasRelationMetadata` to dbt/artifacts
    
    * Move `SourceDefinitionMandatory` to dbt/artifacts
    
    * Move the data parts of `SourceDefinition` to dbt/artifacts
    
    Something interesting here is that we had to override the `freshness`
    property. We had to do this because if we didn't we wouldn't get the
    functional parts of `FreshnessThreshold`, we'd only get the data parts.
    
    Also of note, the `SourceDefintion` has a lot of `@property` methods that
    on other classes would be actual attribute properties of the node. There is
    an argument to be made that these should be moved as well, but thats perhaps
    a separate discussion.
    
    Finally, we have not (yet) moved `NodeInfoMixin`. It is an open discussion
    whether we do or not. It seems primarily functional, as a means to update the
    source freshness information. As the artifacts primarily deal with the shape
    of the data, not how it should be set, it seems for now that `NodeInfoMixin`
    should stay in core / not move to artifacts. This thinking may change though.
    
    * Refactor `from_resource` to no longer use generics
    
    In the next commit we're gonna add a `to_resource` method. As we don't
    want to have to pass a resource into `to_resource`, the class itself
    needs to expose what resource class should be built. Thus a type annotation
    is no longer enough. To solve this we've added a class method to BaseNode
    which returns the associated resource class. The method on BaseNode will
    raise a NotImplementedError unless the the inheriting class has overridden
    the `resouce_class` method to return the a resource class.
    
    You may be thinking "Why not a class property"? And that is absolutely a
    valid question. We used to be able to chain `@classmethod` with
    `@property` to create a class property. However, this was deprecated in
    python 3.11 and removed in 3.13 (details on why this happened can be found
    [here](python/cpython#89519)). There is an
    [alternate way to setup a class property](python/cpython#89519 (comment)),
    however this seems a bit convoluted if a class method easily gets the job
    done. The draw back is that we must do `.resource_class()` instead of
    `.resource_class` and on classes implementing `BaseNode` we have to
    override it with a method instead of a property specification.
    
    Additionally, making it a class _instance_ property won't work because
    we don't want to require an _instance_ of the class to get the
    `resource_class` as we might not have an instance at our dispossal.
    
    * Add `to_resource` method to `BaseNode`
    
    Nodes have extra attributes. We don't want these extra attributes to
    get serialized. Thus we're converting back to resources prior to
    serialization. There could be a CPU hit here as we're now dictifying
    and undictifying right before serialization. We can do some complicated
    and non-straight-forward things to get around this. However, we want
    to see how big of a perforance hit we actually have before going that
    route.
    
    * Drop `__post_serialize__` from `SourceDefinition` node class
    
    The method `__post_serialize__` on the `SourceDefinition` was used for
    ensuring the property `_event_status` didn't make it to the serialized
    version of the node. Now that resource definition of `SourceDefinition`
    handles serialization/deserialization, we can drop `__post_serialize__`
    as it is no longer needed.
    
    * Merge functional parts of `components` into their resource counter parts
    
    We discussed this on the PR. It seems like a minimal lift, and minimal to
    support. Doing so also has the benefit of reducing a bunch of the overriding
    we were previously doing.
    
    * Fixup:: Rename variable `name` to `node_id` in `_map_nodes_to_map_resources`
    
    Naming is hard. That is all.
    
    * Fixup: Ensure conversion of groups to resources for `WritableManifest`
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests