Forwards better plugins

[pslacerda]

My initial attempt to improve the plugin command extensions.

Related to: #347

[pslacerda]

I want to introduce https://circleci.com/ into the build/test/distribute pipeline to do automated tests on commit. What do you think?

[pslacerda]

I noticed that you guys did lots of approvals today. My plan is to update pymol-scripts-repo as soon as possible to Python 3.

The unique drawback I think is a small cost of *args and **kwargs handling.

[JarrettSJohnson]

Hi Pedro, I'll give it a couple of tests soon, but I'm a bit tied up for the next couple of days.

For the CircleCI proposal, we're moving toward Github Actions instead which does build and automate tests already. Any reason to not just stick with that?

As far as the pymol-scripts-repo goes, that repo is community-managed and isn't managed by Schrodinger, but I'm sure your Py 2->3 proposal would probably be welcomed there since we already have made that transition here.

[pslacerda]

I hadn't knew about Github Actions. How do you setup the environment?

I'm running

./install-prefix/bin/pymol -ckqy testing/testing.py --run testing/tests/api/test_commanding.py

Is there an easier method?

[speleo3]

Is there an easier method?

If you have installed PyMOL into a virtual environment, then this should also work:

cd testing
python -m pytest tests/api/test_commanding.py

[pslacerda]

The only plugin that have *args or **kwargs on command declaration is https://github.com/Pymol-Scripts/Pymol-script-repo/blob/ab0762d7cbf5fb004c28791b213ebfc259498345/spectrumbar.py#L9.

Currently this PR accept *args and **kwargs but without typing.

[JarrettSJohnson]

    def test_declare_command_type_return(self):
        @cmd.declare_command
        def func() -> int:
            return 1
        assert func() == 1

fails with

        if function.__code__.co_argcount != len(function.__annotations__):
>           raise Exception("Messy annotations")
E           Exception: Messy annotations

since I'm assuming the return type annotation isn't considered.

[pslacerda]

Yes, I missed the return type.

[pslacerda]

All plugins I read from https://github.com/Pymol-Scripts/Pymol-script-repo/ would works with this API declaration. It isn't so comprehensible because miss types as List or Any. But how it works for many (all?) cases, I think it's good enough.

[JarrettSJohnson]

Thanks, we're going to address the pending PRs after 3.0.

[JarrettSJohnson]

@TstewDev @speleo3 any additional comments regarding this PR?

[TstewDev]

I think this looks good to me! I had a few questions/comments, but these portions of the code that appear to mirror extend and I value preserving this symmetry over any minor changes.

I really like the flexibility of bool! You could in theory test more permutations of these bool input combinations (though I don't really think this is necessary).

We should make sure this is well documented and easy to use for anyone to use that isn't super familiar with PyMOL. Specifically, I think it's worth explaining that declare_command can be used with a single argument, where name is actually the function.

I'm also not sure that the gitignore changes need to be included with this PR. Not that they really cause any harm, but they're not critical to this review and leave out a few I would actually like to add.

Finally, super minor but could you add a blank line to the end of the commanding.py test file?

All in all, great addition!

[pslacerda]

Thank you, Stwart, I really enjoyed to develop this code!

Was thinking that we could add support to List[str], List[int], etc, because I'm almost sure I already saw some code at the wild passing lists as str and parsing it with str.split. Don't implementing it isn't of any concern because the developer could easily work around parsing a str, like as said.

I also want to improve the help command parsing this declare_command and prepend some automatic usage message before the command docstring.

Where the documentation goes so I can start writing in it? My opinion is that it should be easy to understand.

The extend resemblance was really an byproduct it being the declare_command influence.

I'd like that PyMOL had a plugin help usage panel to explore documentation, and I'm willing to develop it as I already have some code to do it somewhere.

Abraços
Pedro

[TstewDev]

There definitely are instances of lists coming from strings that must be parsed and I think it's important to encourage a consistent "design language" for this type of input (i.e. if that's how we want lists to be entered, then that's how they should always be entered).

I agree that some examples would be really helpful along with a clear docstring. This is a must before really even worrying where else we should add documentation for this.

The help tab of the plugin manager links to https://pymolwiki.org/index.php/PluginArchitecture but @speleo3 would probably have the most informed opinion with regard to where documentation for this would be the most helpful.

I don't remember if the existing plugin manager is an incentive only feature or not but I would say this should be added to open-source in some capacity rather than developing a separate "plugin help panel".

[pslacerda]

Currently lists are encoded as a string with spaces or commas.

my_cmd "a, b, c, d"
# or
my_cmd a b c d

But it would be better to be parsed with shlex: Here "b c" is a single list item.

my_cmd a "b c" d

According to IA, a Python list as string passed to eval would work. I don't think there would be any security issues except for sharing .pml files, maybe. But it's a rather unpratical syntax to type. This way it would be trivial to support List, Dict or whatever.

def my_cmd(lst):
    assert eval(lst) == ["a", "b", "c", "d"]

There is also the option to parse the AST (I really enjoy handle code with AST) without eval.

I just checked how to detect generic types like List[int] and found this code. But I'm unsure how to handle lists yet.

About the examples, you can check drugpy new version. It has all the currently supported types.

Seems that the propietary and open-source Plugin Manager are the same:

[pslacerda]

My opinion is that we can test for each individual type like List[str], List[int], etc (e.g. tp == List[str]). But it would work different in Python and .pml scritps, except if there is an way to detect if the command come from a .pml/command line or a Python script. There is an way?

[pslacerda]

ping @TstewDev

[TstewDev]

@pslacerda Please forgive the delay on getting back to this!

scene_order is a good example of what I was thinking of for the option to either provide a list or space separated string. I think this is reasonable to have as a standard and thus your current implementation is fine. It's not worth worrying about the a "b c" d example currently since in reality, we try to avoid including spaces in names (substituting them for underscores).

I don't think it's necessary to use eval but I don't really have that strong of an opinion, it just feels like unnecessary complexity. I also don't know enough about AST to have an strong opinion on that either. It seems better but again may make this more complicated than it needs to be.

drugpy seems like a good example with solid documentation and examples. It does appear that the plugin manager is the same.

Finally, for the python vs pml/command component, PyMOL itself needs to know where the commands are coming from in general. This appears to be handled by the parser itself in parser.py but I'm not sure how this normally interacts with plugins. I would assume that this would be treated like any other command but I don't know if there is any way determining the source without using the parser.

[pslacerda]

Now I'm checking the stacktrace with the traceback module if the last call before current is parser.py. It seems to work.

I need to capture errors from cmd.do, but pytest's capsys doesn't works on PyMOLTestCase tests, so I did pytest style test funcions.

[pslacerda]

@TstewDev

scene_order is a good example of what I was thinking of for the option to either provide a list or space separated string. I think this is reasonable to have as a standard and thus your current implementation is fine. It's not worth worrying about the a "b c" d example currently since in reality, we try to avoid including spaces in names (substituting them for underscores).

It was trivial to use shlex to parse lists, so I go for it.

drugpy seems like a good example with solid documentation and examples.

Thank you, I'm glad you liked its code.

Finally, for the python vs pml/command component, PyMOL itself needs to know where the commands are coming from in general. This appears to be handled by the parser itself in parser.py but I'm not sure how this normally interacts with plugins. I would assume that this would be treated like any other command but I don't know if there is any way determining the source without using the parser.

Based on your observation, I simply check if he previous call was from parser.py, it seems to always work.

[TstewDev]

I still don't think you should include your gitignore changes in this review, but it otherwise looks good to me!

[pslacerda]

I couldn't make *args and **kwargs works in a sane way, so I'm giving up to parse commands like spectrumbar .

It could always be declared with cmd.extend.

I'll remove my .gitignore, thank you.

[JarrettSJohnson]

lgtm. Let me know when this is ready to go.

[pslacerda]

Seems done to me! I don't see any further improvements in this PR, except if you have so.

Some of what I imagined I didn't make, like support for enums (it is still possible), or like autocomplete arguments (seems impossible to do it in a reliable way because completions is tied to a specific argument by order, which is subject to change when passing as keyargs, e.g. f(second=1, first=2).

I'm also imagining a help window where users can search for commands and view the command declaration and formatted docstring.

[JarrettSJohnson]

Thanks! Feel free to open another PR if/when you want to augment it.

[pslacerda]

Where should I document this feature? Some possible options are:

• https://pymolwiki.org/index.php/PluginArchitecture
• https://pymolwiki.org/index.php/Extend (new page declare_command)
• https://pymol.org/dokuwiki/doku.php?id=api:cmd:extend (new page declare_command)

[JarrettSJohnson]

New page on pymolwiki.org is fine. We're not really recommending use of the dokuwiki on pymol.org any/for much longer.