Skip to content

Override perform_get() and perform_set() without a subclass#30

Open
klanclos wants to merge 15 commits intomainfrom
getters_setters
Open

Override perform_get() and perform_set() without a subclass#30
klanclos wants to merge 15 commits intomainfrom
getters_setters

Conversation

@klanclos
Copy link
Contributor

@klanclos klanclos commented Mar 4, 2026

This pull requests enables the developer to specify how GET and SET requests will be handled on a per-item basis without requiring the use of custom subclasses of mktl.Item.

@klanclos klanclos marked this pull request as ready for review March 4, 2026 02:45
@klanclos klanclos requested review from a team March 4, 2026 02:45
@baileyji
Copy link

baileyji commented Mar 4, 2026

Is there documentation on what constitutes a valid performer method? Some things I immediately wonder about:

  • Can it assume an asyncio event loop is running? If so can it use it?
  • How long can it take?
  • Does it need to be reentrant?
  • Might it be called concurrently?
  • How can a daemon author say, in essence, "performer method X is blocked during the execution of performer method Y"?
  • What is the (implicit) contract around errors and exceptions it should raise, if any?
  • Are there restrictions on the return values?
  • I can envision many commands where a get query needs arguments, especially for engineering/debug commands. It doesn't look like there is any provision for that.

Also I believe that Daemon._perform_get_wrapper is superfluous code as the performer will either be a function or a bound method of its class instance.

A quick example:

from typing import Callable

class Bar:
    def __init__(self, name:str):
        self.name = name
    
    def core_method(self, *args):
        print(f"Core method of {repr(self)} name={self.name} called with args: {args}")


class Foo:
    def __init__(self, method:Callable, b:str):
        self.anothers_method = method
        self.b = b
    
    def call_anothers(self, arg):
        self.anothers_method(arg, self.b)
        
    def core_method(self, *args):
        print(f"Core method of {repr(self)} {self.b} called with args: {args}")
        

x = Bar('external')
y = Foo(x.core_method, 'bval, foo instance')

y.call_anothers('some arg to call_anothers')
y.core_method('some arg to y core method')

and a deep dive is here https://docs.python.org/3/howto/descriptor.html

@klanclos
Copy link
Contributor Author

klanclos commented Mar 4, 2026

@baileyji The main thing I'm trying to accomplish with the wrapper method is to allow the implementation to define/provide a performer method that doesn't receive an Item instance as the first argument. If I used something like types.MethodType to replace the Item.perform_set reference, whatever I used there would receive 'self' (the mktl.Item instance) as the first argument.

I believe your example code effectively does the same thing as what's in the pull request; I store a reference to the externally provided method, regardless of whether it is a function or an instance method, and replace Item.perform_set with Item._perform_set_wrapper so the rest of the handling chain remains unaware.

I'll add language to the docstrings about the expected return values, I see that's missing. Thanks for the heads-up.

With respect to how the request is executed, I will add language to the docstrings for Item.perform_get() and Item.perform_set(), since it's relevant there as well-- and the documentation for the other methods should all be landing in the same place so it's only described once.

I can flesh out a similar structure for handling the raw request, in addition to this mechanism for overriding perform_get() and perform_set(); I'll look into that.

@klanclos klanclos marked this pull request as draft March 4, 2026 20:28
@klanclos
Copy link
Contributor Author

klanclos commented Mar 5, 2026

This pull request is starting to get a bit wordy with the families of methods to handle the various ways of adding and setting requests, but I think it's reasonably complete at this point.

@klanclos klanclos marked this pull request as ready for review March 5, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants