Not add base classes to its internal `text_command_classes` and etc variables #2175

evandrocoan · 2018-01-31T23:44:00Z

Summary

File: D:/User/Programs/SublimeText/3143/sublime_plugin.py
101: def reload_plugin(modulename):
102:     print("reloading plugin", modulename)
103: 
104:     if modulename in sys.modules:
105:         m = sys.modules[modulename]
106:         unload_module(m)
107:         m = imp.reload(m)
108:     else:
109:         m = importlib.import_module(modulename)
110: 
111:     module_plugins = []
112:     on_activated_targets = []
113:     module_view_event_listener_classes = []
114:     for type_name in dir(m):
115:         try:
116:             t = m.__dict__[type_name]
117:             if t.__bases__:
118:                 is_plugin = False
119:                 if issubclass(t, ApplicationCommand):
120:                     application_command_classes.append(t)
121:                     is_plugin = True
122:                 if issubclass(t, WindowCommand):
123:                     window_command_classes.append(t)
124:                     is_plugin = True
125:                 if issubclass(t, TextCommand):
126:                     text_command_classes.append(t)
127:                     is_plugin = True
128:

When you do these if issubclass(t, TextCommand), if issubclass(t, WindowCommand), etc. You also add the base class LspTextCommand from an inheritance chain like this:

class LspTextCommand(TextCommand):
    def __init__(self, view):

class LspFormatDocumentCommand(LspTextCommand):
    def __init__(self, view):
        super(LspFormatDocumentCommand, self).__init__(view)

So far no problems. But if the base class LspTextCommand defines a different constructor interface like:

class LspTextCommand(TextCommand):
    def __init__(self, view, different):

And as Sublime Text added this base class LspTextCommand to text_command_classes, it will cause this other line:

File: D:/User/Programs/SublimeText/3143/sublime_plugin.py
195: def create_text_commands(view_id):
196:     view = sublime.View(view_id)
197:     cmds = []
198:     for class_ in text_command_classes:
199:         cmds.append(class_(view))
200:     return cmds

To throw a nice error:

Traceback (most recent call last):
  File "F:\SublimeText\sublime_plugin.py", line 199, in create_text_commands
    cmds.append(class_(view))
TypeError: __init__() missing 1 required positional argument: 'different'

Therefore, Sublime Text should not add base classes to its internal text_command_classes variable.

Expected behavior

from inspect import signature
                ...
                if issubclass(t, ApplicationCommand) and len(signature(t.__init__).parameters) == 2:
                    application_command_classes.append(t)
                    is_plugin = True
                if issubclass(t, WindowCommand) and len(signature(t.__init__).parameters) == 2:
                    window_command_classes.append(t)
                    is_plugin = True
                if issubclass(t, TextCommand) and len(signature(t.__init__).parameters) == 2:
                    text_command_classes.append(t)
                    is_plugin = True

This will also fix: #2066 Commands are not propagated when any TextCommand with invalid init signature is defined

On this commit, I push an example of this bug:

sublimelsp/LSP@b0f6de3#diff-63d08c674fd26f58e74e2dbf43b3dd94

Environment

Operating system and version:
- Windows 10 build 15063 x64
- Mac OS ...
- Linux ...
Monitor:
- Resolution 1920x1080
- dpi_scale used in ST 1.0
Sublime Text:
- Build 3143
- 32 bit

The text was updated successfully, but these errors were encountered:

FichteFoll · 2018-02-01T00:00:38Z

To be frank: no. The heuristic to determine a "base class" seems arbitrary and will only fit for your particular use case of a different function signature in __init__. Why do you need that in the first place?

The proper way to define a "base class" is to do it as a mixin, i.e. do not derive a command class and for the actual command use multiple inheritance. Then you can do whatever you must inside your class's __init__ and either invoke the mixin's __init__ with the additional parameter first (if you derive from it first) or the command's __init__. Or just do whatever you're doing with that init param in a different function that you call explicitly.

evandrocoan · 2018-02-01T00:29:15Z

The heuristic to determine a "base class" seems arbitrary

It is definitely not arbitrary, and it dictated by Sublime Text instantiation of the command classes on:

File: D:/User/Programs/SublimeText/3143/sublime_plugin.py
199:         cmds.append(class_(view))

Here it is instantiating the command classes and forcing them to have exactly 2 parameters (self and view).

The proper way to define a "base class" is to do it as a mixin

I cannot do easily a mixing which needs to call super without adding 2 constructors call on the derived class. To illustrate it, consider the following:

from sublime_plugin import TextCommand

class LspTextCommand(object):
    def __init__(self, capability):
        self.capability = capability

class LspFormatDocumentCommand(TextCommand, LspTextCommand):
    def __init__(self, view):
        TextCommand.__init__(view)
        LspTextCommand__init__('other_stuff')

class LspSymbolDefinitionCommand(LspTextCommand):
    def __init__(self, view):
        TextCommand.__init__(view)
        LspTextCommand.__init__('more_other_stuff')

This forces on all derived classes to do 2 super constructors calls TextCommand and LspTextCommand. But inhering TextCommand on LspTextCommand, I can do everything just with one constructor call in all my derived classes:

from sublime_plugin import TextCommand

class LspTextCommand(TextCommand):
    def __init__(self, view, capability):
        super(LspTextCommand, self).__init__(view)
        self.capability = capability

class LspFormatDocumentCommand(LspTextCommand):
    def __init__(self, view):
        super(LspFormatDocumentCommand, self).__init__(view, 'other_stuff')

class LspSymbolDefinitionCommand(LspTextCommand):
    def __init__(self, view):
        super(LspFormatDocumentCommand, self).__init__(view, 'more_other_stuff')

Unless I am missing something?

Also, Sublime Text should not add any classes to its internal lists if it cannot instantiate them later on:

File: D:/User/Programs/SublimeText/3143/sublime_plugin.py
199:         cmds.append(class_(view))

Otherwise, some even plugin can just break most things with #2066 or and not allow me to create the inheritance just above.

Do you see some flaw in this? Do you still think Sublime Text should keep blinding adding everything which just happens to extend ApplicationCommand, WindowCommand and TextCommand when it clearly one can instantiate classes requiring exacly 2 parameters self and view?

Why do you think it is random to require Sublime Text to load only classes he can instantiate?

Why do you want to Sublime Text to be loading user custom bases classes he cannot instantiate?

This is not a monster fix. This is a simple if checking which is clearly not random. Do you still not understanding why not load them?

evandrocoan · 2018-02-01T00:32:40Z

Or just do whatever you're doing with that init param in a different function that you call explicitly.

    def __init__(self, view, capability='', last_check=lambda: True):

Defining all the other parameters except self and view other within default values is causing Sublime Text to instantiating an useless object instead of throwing an exception when running:

File: F:/SublimeText/sublime_plugin.py
198:     for class_ in text_command_classes:
199:         cmds.append(class_(view))

FichteFoll · 2018-02-01T00:41:14Z

from sublime_plugin import TextCommand

class LspTextCommand(object):
    def __init__(self, view, capability):
        super().__init__(view)  # this will call TextCommand.__init__
        self.capability = capability

class LspFormatDocumentCommand(LspTextCommand, TextCommand):  # the order is important here!
    def __init__(self, view):
        super().__init__(view, 'other_stuff')  # this will call LspTextCommand.__init__

class LspSymbolDefinitionCommand(LspTextCommand, TextCommand):
    def __init__(self, view):
        super().__init__(view, 'more_other_stuff')

https://docs.python.org/3/tutorial/classes.html#multiple-inheritance

evandrocoan · 2018-02-01T00:52:05Z

Well, I did this:

class LspFormatDocumentCommand(TextCommand, LspTextCommand):

And said, # the order is not important here!

FichteFoll · 2018-02-01T00:59:00Z

By the way, if the data you want to specify in, say, capability, is static, I would rather use a class attribute and override that in the subclasses.

from sublime_plugin import TextCommand

class LspTextCommand(object):
    capability = None
    
    # … some methods doing stuff with `cls.capability` or `self.capability`

class LspFormatDocumentCommand(LspTextCommand, TextCommand):  # the order is *still* important here if LspTextCommand overrides any of TextCommand's methods
    capability = 'other_stuff'

class LspSymbolDefinitionCommand(LspTextCommand, TextCommand):
    capability = 'more_other_stuff'

Not add base classes to its internal `text_command_classes` and etc variables sublimehq/sublime_text#2175

evandrocoan · 2018-02-01T01:22:31Z

Thanks, yes, I did not have though about doing that, but in this case, the capability attribute is only used by base class methods, so would be strange having the derived class defining it, but not using it anywhere.

FichteFoll · 2018-02-01T02:03:47Z

So, can we close this issue? I believe with what I showed, the proposal is simply unnecessary.

would be strange having the derived class defining it, but not using it anywhere.

I don't see an issue here. That's just polymorphism.

evandrocoan · 2018-02-01T02:10:35Z

I would close the as a Duplicate of:

Commands are not propagated when any TextCommand with invalid __init__ signature is defined #2066 Commands are not propagated when any TextCommand with invalid init signature is defined

Because they have the same fix.

I don't see an issue here

Well, when I see something like it, I brain just trigger delete this unused variable, but then later you find out it is used on the base class.

commit 09d5f319803e4ea08cfc70001a898a3c471d480b Author: Tom van Ommeren <tom.vanommeren@gmail.com> Date: Mon Feb 5 22:54:50 2018 +0100 Small fixes before merging - don't cache client in hover.py (risk to client restart functionality) - improvement: show diagnostics hovers even if no hoverProvider capability. - remove unneeded event param additions in main.py - restore is_at_word in definition.py commit 36e339b Author: evandrocoan <evandrocoan@hotmail.com> Date: Mon Feb 5 05:53:09 2018 -0200 Passed event to is_at_word(self.view, event) commit ac5e228 Author: evandrocoan <evandrocoan@hotmail.com> Date: Mon Feb 5 00:38:18 2018 -0200 Added missing return on plugin/symbols.py commit 45170ab Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 22:08:22 2018 -0200 TypeError: is_enabled() got an unexpected keyword argument 'point' commit 1a8cd9a Merge: 887a480 bf6a5b4 Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 20:12:10 2018 -0200 Merge branch 'master' into smart_context_menu commit 887a480 Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 20:09:12 2018 -0200 Moved capability from LspTextCommand constructor to has_client_with_capability(). commit c790187 Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 19:52:15 2018 -0200 Fixed clients.py:54:1: E302 expected 2 blank lines, found 1 commit 12bd209 Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 19:49:37 2018 -0200 Fixed hover.py:51: error: Name 'client_for_view' is not defined commit c314e37 Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 19:22:21 2018 -0200 Implemented is_enabled() on all derived classes. commit 6b23fec Merge: 5519535 3bc9cca Author: evandrocoan <evandrocoan@hotmail.com> Date: Sun Feb 4 15:39:24 2018 -0200 Merge branch 'master' into smart_context_menu commit 5519535 Merge: 544d487 b6a3b9d Author: evandrocoan <evandrocoan@hotmail.com> Date: Sat Feb 3 20:19:19 2018 -0200 Merge branch 'master' into smart_context_menu commit 544d487 Author: evandrocoan <evandrocoan@hotmail.com> Date: Fri Feb 2 14:18:43 2018 -0200 Replaced Python 2 super(...) call style by Python 3 super() commit 598a0f6 Merge: e4e5929 7ef8268 Author: evandrocoan <evandrocoan@hotmail.com> Date: Fri Feb 2 14:09:52 2018 -0200 Merge branch 'master' into smart_context_menu commit e4e5929 Author: evandrocoan <evandrocoan@hotmail.com> Date: Thu Feb 1 23:39:18 2018 -0200 Removed the mixing to simply code reading, writing and remove the mypy error: mypy -p plugin clients.py:26: error: Too many arguments for "__init__" of "object" commit 0a397e3 Author: evandrocoan <evandrocoan@hotmail.com> Date: Thu Feb 1 19:30:02 2018 -0200 Fix clients.py 'sublime_plugin.TextCommand' imported but unused commit af5bb11 Author: evandrocoan <evandrocoan@hotmail.com> Date: Wed Jan 31 23:06:16 2018 -0200 Created a mixing with TextCommand and LspTextCommand, issue: Not add base classes to its internal `text_command_classes` and etc variables sublimehq/sublime_text#2175 commit b0f6de3 Author: evandrocoan <evandrocoan@hotmail.com> Date: Wed Jan 31 21:52:10 2018 -0200 Added is_enabled() to LspTextCommand base class. commit 18d080a Author: evandrocoan <evandrocoan@hotmail.com> Date: Wed Jan 31 19:25:04 2018 -0200 Renamed LspContextMenu to LspTextCommand on configurations.py commit a192796 Author: evandrocoan <evandrocoan@hotmail.com> Date: Tue Jan 30 20:35:15 2018 -0200 Fixed The command "mypy -p plugin" exited with 0. 1.94s$ flake8 --exclude=./boot.py ./plugin/code_actions.py:2:1: F401 'sublime_plugin' imported but unused ./plugin/definition.py:2:1: F401 'sublime_plugin' imported but unused ./plugin/formatting.py:1:1: F401 'sublime_plugin' imported but unused ./plugin/references.py:3:1: F401 'sublime_plugin' imported but unused ./plugin/rename.py:1:1: F401 'sublime_plugin' imported but unused ./plugin/symbols.py:1:1: F401 'sublime_plugin' imported but unused commit 31f435c Author: evandrocoan <evandrocoan@hotmail.com> Date: Tue Jan 30 20:26:02 2018 -0200 Hide Context.sublime-menu when working on projects without language servers

evandrocoan mentioned this issue Feb 1, 2018

Hide Context.sublime-menu when working on projects without language sublimelsp/LSP#267

Closed

evandrocoan added a commit to evandrocoan/LSP that referenced this issue Feb 1, 2018

Created a mixing with TextCommand and LspTextCommand, issue:

e7191d6

Not add base classes to its internal `text_command_classes` and etc variables sublimehq/sublime_text#2175

evandrocoan added a commit to evandrocoan/LSP that referenced this issue Feb 1, 2018

Created a mixing with TextCommand and LspTextCommand, issue:

af5bb11

Not add base classes to its internal `text_command_classes` and etc variables sublimehq/sublime_text#2175

evandrocoan closed this as completed Feb 1, 2018

evandrocoan mentioned this issue Feb 1, 2018

Commands are not propagated when any TextCommand with invalid __init__ signature is defined #2066

Closed

FichteFoll added R: wontfix C: API labels Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not add base classes to its internal `text_command_classes` and etc variables #2175

Not add base classes to its internal `text_command_classes` and etc variables #2175

evandrocoan commented Jan 31, 2018 •

edited

FichteFoll commented Feb 1, 2018

evandrocoan commented Feb 1, 2018

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

Not add base classes to its internal text_command_classes and etc variables #2175

Not add base classes to its internal text_command_classes and etc variables #2175

Comments

evandrocoan commented Jan 31, 2018 • edited

Summary

Expected behavior

Environment

FichteFoll commented Feb 1, 2018

evandrocoan commented Feb 1, 2018

evandrocoan commented Feb 1, 2018 • edited

FichteFoll commented Feb 1, 2018 • edited

evandrocoan commented Feb 1, 2018 • edited

FichteFoll commented Feb 1, 2018 • edited

evandrocoan commented Feb 1, 2018 • edited

FichteFoll commented Feb 1, 2018 • edited

evandrocoan commented Feb 1, 2018 • edited

Not add base classes to its internal `text_command_classes` and etc variables #2175

Not add base classes to its internal `text_command_classes` and etc variables #2175

evandrocoan commented Jan 31, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited

FichteFoll commented Feb 1, 2018 •

edited

evandrocoan commented Feb 1, 2018 •

edited