classification
Title: cmd.Cmd.get_help() implementation can't see do_*() methods added dynamically by setattr()
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Błażej Michalik, catherinedevlin, remi.lapeyre, rhettinger, tleonhardt, tuxtimo
Priority: normal Keywords: patch

Created on 2016-11-10 11:14 by Błażej Michalik, last changed 2019-01-14 21:35 by rhettinger.

Pull Requests
URL Status Linked Edit
PR 8015 open remi.lapeyre, 2018-06-29 20:57
Messages (15)
msg280502 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2016-11-10 11:14
The issue here seems to originate from the implementation of Cmd.get_names():

    def get_names(self):                                   
        # This method used to pull in base class attributes
        # at a time dir() didn't do it yet.                
        return dir(self.__class__)                         

Changing it to dir(self) would make dynamical adding helpstrings for commands possible.
msg318356 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-06-01 01:29
ISTM the only way dir(self) would make a difference would be if functions we being assigned to instances rather than the class.   Also, it's uncleaar why setattr() is at issue -- it works the same way as regular attribute assignment using the dot operator.
msg319819 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2018-06-17 15:55
Sorry for not describing this one particularly well.

There is nothing wrong with setattr() here, that wasn't the point.
Given the code below (py3, lambdas used for brevity):

    # coding: utf-8
    from cmd import Cmd

    class MyCmd(Cmd):
        def do_documented_at_definition(self):
            """ This one is documented with docstring. """
            pass
    
        def do_documented_afterwards(self):
            _ = 'This one will be documented afterwards, with help_<...>() method.'
        
    cli = MyCmd()
    cli.do_new_command = lambda *_: print("I'm new here")
    cli.help_documented_afterwards = lambda *_: print("I'm documenting")
    cli.cmdloop()


1. When one types in "help":
    1.1 there is no mention of "new_command" in the output, even though the command works.
    1.2 "documented_afterwards" command is being presented as having no documentation at all, but after typing "help documented_afterwards", it turns out that it is not the case.
2. There is no completion for "new_command", i.e. typing in "new_" and pressing tab key will not make it complete to "new_command".

I don't remember what was the use-case for this at the time (probably had something to do with disabling / enabling certain commands at runtime), but the implementation seemed trivial enough that we couldn't see justification for not doing it this way, hence the report.
msg320740 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2018-06-29 20:58
Hi Błażej Michalik, can you confirm the attached patch fixed the issue for you?
msg320787 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2018-06-30 15:25
Yes, the proposed patch fixes all the problems that I pointed out in the last comment.
msg320791 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-06-30 16:46
Błażej, why is there a need to attach a function to the cli instance rather than the MyCmd class (which would be the norm for Python)? 

    from cmd import Cmd

    class MyCmd(Cmd):
        def do_documented_at_definition(self, arg):
            """ This one is documented with docstring. """
            print('dad', arg)

        def do_documented_afterwards(self, arg):
            print('documented afterwards', arg)
        
    cli = MyCmd()

    def do_new_command(self, arg):
        print('new command', arg)

    MyCmd.do_new_command = do_new_command

    def help_documented_afterwards(*args):
        print("I'm documenting", args)

    MyCmd.help_documented_afterwards = help_documented_afterwards
        
    cli.cmdloop()

Sample session:

    (Cmd) help

    Documented commands (type help <topic>):
    ========================================
    documented_afterwards  documented_at_definition  help

    Undocumented commands:
    ======================
    new_command

    (Cmd) help documented_at_definition
     This one is documented with docstring. 
    (Cmd) help documented_afterwards
    I'm documenting (<__main__.MyCmd object at 0x10eaf0c88>,)
    (Cmd) documented_at_definition x
    dad x
    (Cmd) documented_afterwards y
    documented afterwards y
    (Cmd) new_command z
    new command z
    (Cmd) help new_command
    *** No help on new_command
msg320812 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2018-06-30 22:27
It was nearly 2 years ago when we needed it, and we avoided the issue by overriding get_names(). I'll have to find the code in which this was used to answer your question.

I guess we could've modified the class definition, however weirdly would that look.

But then again, shouldn't the commands that are runnable from inside .cmdloop() be auto-completable and visible in 'help'? IMHO it does make some intuitive sense, or at least it seems less surprising.
msg320849 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-02 00:52
Okay, I'll mark this as on-hold until you find the relevant code.

I'm not sure why you think modifying the class definition would look weird.  That is the normal and intended place to put methods.  Likely, the only reason that you had any success at all when attaching a function to an instance was that it didn't need access to either "self" or "class".
msg320994 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2018-07-03 17:59
I found it.

Our app had CLI that was so vast, that it didn't made sense to put all of the 'do_xyz' methods into the same class that would run the interface internals. Instead, we had a child Cmd class that had a "add_command(self, command, func, helpstring=None)" method. 

That method would then perform an assignment with setattr() calls: setattr(self, "do_" + command, func). That's why the title of this issue has the innocent setattr() mentioned. I didn't knew at the time, whether or not the usage of it was important to the issue.

It made sense to us back then, since we had multiple components waiting for the user to interact with them, that mostly didn't had much of anything to do with each other. 
It also made keeping track of the commands much easier. Each component had its own set of command, callback and helpstring triplets, and putting them together into the Cmd class was a responsibility of a completely separate code.

It looked cleaner that way.

When I was talking about "modifying class definition", I had a "OurCmd().__class__.do_xyz = callback" situation in mind. I think that it makes it behave in the same way as with the patch, as long as there is only one OurCmd instance.
msg321142 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-05 23:34
> That method would then perform an assignment with setattr() calls: setattr(self, "do_" + command, func).

Could this have been done with:  setattr(self.__class__, "do_" + command, func)?

I'm concerned that attaching functions to the instance is fighting the Python norms and the design of the module.  It seems like a can of worms.
msg321305 - (view) Author: Błażej Michalik (Błażej Michalik) Date: 2018-07-09 09:20
It seems to me as if the current implementation forces the user to violate SRP, but I'm not going to argue about that. I don't feel equipped well enough to judge.

> Could this have been done with:  setattr(self.__class__, "do_" + command, func)?

Isn't that worse than doing that on an instance? It will work as long as there's only one.
msg321354 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-07-10 04:42
>> Could this have been done with:  
>> setattr(self.__class__, "do_" + command, func)?

> Isn't that worse than doing that on an instance? It will work 
> as long as there's only one.

Why not have multiple classes instead of multiple instances?  Then use class composition to combine the components.

FWIW, I just looked through the Cmd2 project (which depends on and extends the Cmd class): https://cmd2.readthedocs.io/en/latest/index.html .  AFAICT, there is no precedent for the approach taken by your project.  This seems to be at odds with the design of module which is class based rather than instance based.

The existing class based design allows class composition which supports needs for separately maintained components.  It also allows regular methods to be dynamically attached to the class or any of its parents after the class is created.   Being regular methods, they would have access to the "self" instance parameter so that the application can be stateful (note that attaching functions to instances loses this advantage).  In Cmd2, the instance is considered the application and only one can be run at a time (it's not really about having multiple distinct instances that vary dynamically and independently of one another).

I've exhausted my triage efforts on this.  It doesn't seem be bug; rather, it is a feature request to take the module in a different direction that seems to be in conflict with its core concept and normal use (and in conflict with Python norms about where to attach methods).  

If you would like to push forward, consider eliciting opinions and analysis from Catherine Devlin and Todd Leonhardt (the maintainers of Cmd2).  If they think it is a good idea to have multiple instances with dynamically attached functions, then this feature request can go forward.
msg321374 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2018-07-10 10:16
Hi Raymond, would updating the attached PR to change https://github.com/remilapeyre/cpython/blob/7c78350f8903f162e5f70ee147c0e97cb1ed5181/Lib/cmd.py#L270 (and others) from `compfunc = getattr(self, 'complete_' + cmd)` to `compfunc = getattr(self.__class__, 'complete_' + cmd)` to have a consistent behavior for the help and the complection ?

Maybe the methods could all rely on `get_names` that return `dir(self.__class__)`, that way, the behavior would be more consistent and Błażej could override this method only to get its desired result.
msg333610 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2019-01-14 11:27
CC-ing Catherine Devlin and Todd Leonhardt so we can close the issue by either accepting or refusing such feature.
msg333643 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-01-14 21:35
> Maybe the methods could all rely on `get_names` that
> return `dir(self.__class__)`,

Please read the previous posts on the subject.  The only reason to do this is if we want the module to explicitly support attaching methods directly to instances.  That practice has disadvantages (no access to self) and it is at odds with the design of the module (nothing we ever intended to explicitly support) and it is outside of usual Python norms (methods attached to classes).  AFAICT, the OP's need could be met using traditional (and supported) techniques such as class composition.  Accordingly, there isn't any need to go down this more unusual path.
History
Date User Action Args
2019-01-14 21:35:21rhettingersetmessages: + msg333643
2019-01-14 11:27:57remi.lapeyresetmessages: + msg333610
2019-01-14 11:25:44remi.lapeyresetnosy: + catherinedevlin, tleonhardt
2018-07-10 10:16:32remi.lapeyresetmessages: + msg321374
2018-07-10 04:42:05rhettingersetmessages: + msg321354
2018-07-09 09:20:50Błażej Michaliksetmessages: + msg321305
2018-07-05 23:54:00rhettingersetresolution: later ->
2018-07-05 23:34:05rhettingersetmessages: + msg321142
2018-07-03 17:59:20Błażej Michaliksetmessages: + msg320994
2018-07-02 00:52:44rhettingersetresolution: later
messages: + msg320849
versions: + Python 3.8, - Python 2.7, Python 3.3, Python 3.4, Python 3.5, Python 3.6, Python 3.7
2018-06-30 22:27:59Błażej Michaliksetmessages: + msg320812
2018-06-30 16:46:32rhettingersetmessages: + msg320791
2018-06-30 15:25:29Błażej Michaliksetmessages: + msg320787
2018-06-29 20:58:15remi.lapeyresetnosy: + remi.lapeyre
messages: + msg320740
2018-06-29 20:57:22remi.lapeyresetkeywords: + patch
stage: patch review
pull_requests: + pull_request7623
2018-06-17 15:55:48Błażej Michaliksetmessages: + msg319819
2018-06-01 01:29:13rhettingersetnosy: + rhettinger
messages: + msg318356
2018-05-31 16:14:49tuxtimosetnosy: + tuxtimo
2016-11-10 11:14:10Błażej Michalikcreate