ENB: An important puzzle re Leo's decorators

[Edward K. Ream]

For the last few days I have been working on #325. The first comment of this issue starts with:

> I want to replace @cmd decorators by by @g.command decorators everywhere, thereby eliminating g.cmd_instance_dict, which is unbearably ugly.

The puzzle

Following this stack overflow post re "self" arguments in decorators, I created the following prototype:

global_commands_dict = {}

def command_method(name):
    """A decorator that creates a Leo command from a method."""
   
    def _wrapper(f):
        """This wrapper is always called with one argument: the wrapped function."""

        def inner_wrapper(self, event):
            """Inner wrapper docstring."""
            return f(self, event)
           
        inner_wrapper.__name__ = f.__name__ # Doesn't work.
        inner_wrapper.__doc__ = f.__doc__  # works.
        return inner_wrapper

    global_commands_dict[name] = _wrapper
    return _wrapper

class Test:

    @command_method('get')
    def get(self, event):
        """docstring for Test.get."""
        g.trace(Test.get.__doc__)
   
event = g.Bunch(c=c)    
Test().get(event)
g.printObj(global_commands_dict, tag='dict')

Running this script gives the following output.

get docstring for Test.get.
dict:
{
  get:<function command_method.<locals>._wrapper at 0x00000170F5D90288>
}

Alas, I have not been able to generalize this apparently successful prototype so that it works in Leo.  There is another possible way forward. See below.

Background

You would think that defining decorators would be easy. pep 318 defines the effect of decorators in the current syntax section:

@dec2
@dec1
def func(arg1, arg2, ...):
     pass

is equivalent to:

def func(arg1, arg2, ...):
    pass

func = dec2(dec1(func))

without the intermediate assignment to the variable func.

This seems straightforward, but binding issues are enough to make my head explode :-)

The argument to any decorator is the method to be decorated. For decorated methods, this an unbound method. The "self" argument is available, but only later, when the bound method gets called. There are also (to me) confounding issues about what the argument lists to the _wrapper and inner_wrapper functions should be.

A failed g.command_name function

I defined g.command_method as follows:

def command_method(name):
   
    def _wrapper(f):
        """f is the wrapped function."""

        def inner_wrapper(self, event=None, *args, **kwargs):
            """Inner wrapper docstring."""
            return f(self, event, *args, **kwargs)

        # inner_wrapper.__name__ = f.__name__  # happens too late to be useful.
        f.is_command = True
        f.command_name = name
        inner_wrapper.__doc__ = f.__doc__  # works.
        return inner_wrapper

    global_commands_dict[name] = _wrapper
    return _wrapper

Alas, this does not work. Using this to define the undo/redo commands does create undo and redo commands, but the inner wrapper is never called. In contrast, defining undo like this does work:

@g.command('undo')
def undo_command(event):
    c = event.get('c')
    if not c:
        return
    c.undoer.undo(event)

Another possibility

Leo already contains per-class and per-file @cmd decorators. At present these cmd functions use g.new_cmd_decorator. For example, LeoBody.cmd is:

def cmd(name):
    """Command decorator for the c.frame.body class."""
    # pylint: disable=no-self-argument
    return g.new_cmd_decorator(name, ['c', 'frame', 'body'])

Another possible solution would be to create per-class cmd methods that use "self" to avoid call g.new_cmd_decorator. Iirc, I tried and failed to do this years ago. It would be best if the new cmd methods were relatively simple :-) In any case, they must not use g.cmd_instance_dict, either directly or indirectly.

The commands and commands2 branches

The commands branch avoids these puzzles by creating top-level function that define all of Leo's commands. I have been using a script to do the dirty work.

The commands2 branch defines the undo and redo commands using top-level methods. Replacing these by @g.command_method decorators applied to the undo/redo methods themselves fails. The commands are not functional. As a result various unit tests fails.

Summary

Your challenge, should you choose to accept it, is to create g.command_method function in leoGlobals.py which can be used in any file as a decorator. Alternatively, you may create a relatively simple template for the various cmd functions/methods.

No solution may use g.cmd_instance_dict, either directly or indirectly. All unit tests must pass :-)

The commands2 branch contains failed experiments. I'll use the code in the commands branch if no reasonable solution to these puzzles can be found. There is no great hurry. I won't merge "commands" into devel any time soon.

Edward

[vitalije]

I have changed your first test script to work. I am not sure if that is all you wanted.

import functools
global_commands_dict = {}

def command_method(name):
    """A decorator that creates a Leo command from a method."""
    def _wrapper(f):
        """This wrapper is always called with one argument: the wrapped function."""

        @functools.wraps(f)

        def inner_wrapper(self, event):
            """Inner wrapper docstring."""
            return f(self, event)

        global_commands_dict[name] = inner_wrapper
        return inner_wrapper

    return _wrapper

class Test:

    @command_method('get')
    def get(self, event):
        """docstring for Test.get."""
        g.trace(Test.get.__doc__)

event = g.Bunch(c=c)
Test().get(event)
g.printObj(global_commands_dict, tag='dict')

Executing it write to the console following:

get docstring for Test.get.
dict:
{

  get:<function Test.get at 0x7f1b980b8b90>
}

Vitalije

[Edward K. Ream]

On Mon, May 4, 2020 at 7:44 AM vitalije <vita...@gmail.com> wrote:

I have changed your first test script to work. I am not sure if that is all you wanted.

Thanks! I'll play with it.

Edward

[Edward K. Ream]

On Mon, May 4, 2020 at 7:44 AM vitalije <vita...@gmail.com> wrote:

> I have changed your first test script to work. I am not sure if that is all you wanted.

It fails as in my previous tests.  In leoUndo.py I used your version of g.command_method:

@g.command_method('undo')
def undo(self, event=None):
    ... rest of code unchanged

And similarly for the redo method.

The unit test @test clone-marked-nodes then failed:

TypeError: inner_wrapper() missing 1 required positional argument: 'event'

So I changed:

def inner_wrapper(self, event):

to

def inner_wrapper(self, event=None):

I've always had a bad feeling this kind of binding hack.

With this change another test failed:

@test paste and undo in headline - at end
...
AssertionError: oops3 got: Test headline abcABC

These are the failures I got yesterday.  I have no idea how to make this work. It feels like drowning in an inch of water...

Edward

[Thomas Passin]

The few times I've written decorators, I've always written the arguments as *args.  That way avoids problems where inner methods/functions are called with/without self or default args. E.g.:

# decorator REQUIRE_MAIN

def REQUIRE_MAIN(procedure):

   def new_proc(*args):
      # args[0] would be the calling instance (i.e., self)
      self = args[0]
      ... # guts of the decorator here

      procedure(*args)
   return new_proc

I don't know if this will help here, but it seems a prudent thing to do, since it finesses issues about what gets passed in.

[Edward K. Ream]

On Mon, May 4, 2020 at 8:44 AM Thomas Passin <tbp1...@gmail.com> wrote:

The few times I've written decorators, I've always written the arguments as *args.  That way avoids problems where inner methods/functions are called with/without self or default args. E.g.:

I did play around with adding both *args and **kwargs. No joy. Perhaps dropping the **kwargs might help.

Bindings within decorators seem way too mysterious.

Edward

[vitalije]

I think I have found the solution. I don't think it is possible to solve the problem just using decorators. The command_method decorator needs some help.

Here is a decorator:

def command_method(name):
    """A decorator that creates a Leo command from a method."""

    def _wrapper(f):
        """This wrapper is always called with one argument: the wrapped function."""

        clsname = f.__qualname__.partition('.')[0]
        @functools.wraps(f)
        def inner_wrapper(self, event=None):
            """Inner wrapper docstring."""
            getinst = globals().get(f'{clsname}_in_c')
            c = self.c
            instance = getinst(c)
            return f(instance, event)

        global_commands_dict[name] = inner_wrapper
        return inner_wrapper

    return _wrapper

It relies on g having a function 'Undoer_in_c' for the commands in leoUndo. For every class that uses this decorator, after the class definition it is necesssary to add the following line:

g.Undoer_in_c = lambda c:c.undoer

Other classes should add similar functions with their own names.

The inner_wrapper uses this function to get the instance for executing command.

I have pushed this code in commands2 branch and all unittests pass.

HTH Vitalije

[Edward K. Ream]

On Mon, May 4, 2020 at 11:28 AM vitalije <vita...@gmail.com> wrote:

I think I have found the solution. I don't think it is possible to solve the problem just using decorators. The command_method decorator needs some help.

Thanks for this work.  I tried to pull your code from commands2. There were some merge conflicts, but I don't see the new code. Did you forget to push?

Edward

[vitalije]

Sorry I did the work in commands not in commands2.

Vitalije

[vitalije]

On Monday, May 4, 2020 at 7:17:04 PM UTC+2, vitalije wrote:

Sorry I did the work in commands not in commands2.

Vitalije

Well no it is not true, I did checkout commands2 and committed my work in commands2, and I did push.

git status

On branch commands2
Your branch is up to date with 'origin/commands2'.

Untracked files:
  (use "git add ..." to include in what will be committed)

        ../config/myLeoSettings.leo
        ../core/qtree-profile.leo
        ../themes/dark/Icons/file_icons/
        ../../outlinediffs.py
        ../../toutlines.py

nothing added to commit but untracked files present (use "git add" to track)

I am on d0e62fbd350b01bc678 now and everything is uptodate.

Vitalije

[Edward K. Ream]

On Mon, May 4, 2020 at 12:21 PM vitalije <vita...@gmail.com> wrote:

Well no it is not true, I did checkout commands2 and committed my work in commands2, and I did push.

The problem is likely on my end. I see your work on the commands2 branch on the github branches page.

Edward

[Edward K. Ream]

On Monday, May 4, 2020 at 12:57:49 PM UTC-5, Edward K. Ream wrote:

The problem is likely on my end. I see your work on the commands2 branch on the github branches page.

I installed your code by hand in the commands branch (not pushed yet). Using @g.command_method on do define the 'undo' and 'redo' commands in leoUndo.py passes pylint tests.  Alas, a unit test fails:

ERROR: runTest (leo.core.leoTest.GeneralTestCase)
@test clone-marked-nodes

----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\leo.repo\leo-editor\leo\core\leoTest.py", line 308, in runTest
    exec(compile(script, scriptFile, 'exec'), d)
  File "C:/leo.repo/leo-editor/leo/test/scriptFile.py", line 20, in <module>
    test(p)
  File "C:/leo.repo/leo-editor/leo/test/scriptFile.py", line 15, in test
    c.undoer.undo()
  File "c:\leo.repo\leo-editor\leo\core\leoGlobals.py", line 203, in inner_wrapper
    instance = getinst(c)
TypeError: 'NoneType' object is not callable

Edward

[vitalije]

I don't see that test. I have opened leo/test/unitTest.leo and pressed Alt-4 all unit tests passed.

Where is this test?

Vitalije

[vitalije]

If you have installed by hand perhaps you've missed to change leoUndo.py ? At the end of the file you should add the line:

g.Undoer_in_c = lambda c:c.undoer

Vitalije

[Edward K. Ream]

On Mon, May 4, 2020 at 1:29 PM vitalije <vita...@gmail.com> wrote:

If you have installed by hand perhaps you've missed to change leoUndo.py ? At the end of the file you should add the line:

g.Undoer_in_c = lambda c:c.undoer

Yes, I did miss that.

I am leaning towards using @g.command to define all commands. Yes, defining top-level functions for all commands isn't as "elegant" as using an in-place decorator, but it has the advantage that I can actually understand the decorator :-)

Furthermore, I am having excellent results the with the conversion script. The script usually "just works". Converting a file takes about 15 minutes, with testing.

Edward

[Edward K. Ream]

On Monday, May 4, 2020 at 1:29:40 PM UTC-5, vitalije wrote:

If you have installed by hand perhaps you've missed to change leoUndo.py ? At the end of the file you should add the line:

g.Undoer_in_c = lambda c:c.undoer 

This looks like a substitute for the entries in g.cmd_instance_dict. As the creator of the puzzle I am inclined to disqualify this solution :-)

Edward

[vitalije]

I didn't like it either. But I don't think that it is possible to solve this purely using decorator functions.

Methods need an instance to work correctly. At the time decorators are executed there are no instances yet. They are executed during the class definition, and instance can't be created before class is defined. Perhaps some magic using meta classes and meta programming can help, but it would be even harder to read and understand than it is now.

Vitalije

[Edward K. Ream]

On Mon, May 4, 2020 at 2:56 PM vitalije <vita...@gmail.com> wrote:

I didn't like it either. But I don't think that it is possible to solve this purely using decorator functions.

I agree.

On my walk today I saw things in a new light. The first comment of #325 starts with:

"I want to replace @cmd decorators by by @g.command decorators everywhere, thereby eliminating g.cmd_instance_dict, which is unbearably ugly."

There are two problems with this statement. First, using @g.command decorators everywhere can be clumsy, as the new code in the commands branch makes clear. The virtue of @cmd is that it puts the decorator next to the code that should be decorated. Using @g.command everywhere is one step forward (simplicity) and one step back (added remoteness).

Second, g.cmd_instance_dict is not, in fact, unbearably ugly. In some ways it's clever and descriptive. Indeed, the only thing the @cmd descriptors know is c. The dict allows per-class cmd descriptors to use the chain of ivars to recover the instance of the desired class.

Summary

I now think that Leo's code base is good as it is. There is no great need to use @g.command. Instead, it makes sense to use @cmd to decorate methods and @g.command to decorate functions. In other words, #325 can be retired, after making clear that:

1. g.cmd_instance_dict is a reasonable solution to a very difficult problem,

2. There is a place for both the @cmd and @g.command decorators.

I am willing to abandon all the work I have done in the command branch. I have enjoyed working on the script that created top-level commands using @g.command. It's good code in the service of a what seems now to be a dubious goal. Looking forward, the script is a model for analyzing and changing code. We can keep this model in mind as we consider even more ambitious changes to Leo's code.

Vitalije, I thank you for your work on @g.command. It has convinced me that all solutions to the stated problem will be difficult.

Edward