Declaring new commands #470

pslacerda1 · 2025-10-09T15:35:47Z

This is a continuation of #448.

I test it with the following command line. It doesn't get collected by the default test runner, I guess, because my tests were written in a pytest parlance.

PYTHONPATH=~/Desktop/pymol-open-source/testing/ pytest tests/api/commanding.py

It is still missing windows support.

JarrettSJohnson · 2025-10-09T17:55:38Z

I'll look closer to these this weekend, but if your tests aren't running move the pytest tests to test_commanding.py. See the other files like test_editing.py for an example.

pslacerda1 · 2025-10-09T21:41:25Z

The tests effectively run with the command line provided:

PYTHONPATH=~/Desktop/pymol-open-source/testing/ pytest tests/api/commanding.py

But it doesn't mix well with other tests yet.

JarrettSJohnson · 2025-10-09T21:47:39Z

Can you try

python testing/testing.py --run testing/tests/api/commanding.py

this is how I normally run individual test files.

JarrettSJohnson · 2025-10-09T21:49:09Z

Either way, I would stick with the convention with what we have (separating the files and putting them in files preceding with test_). The CI will run all pytests and unittests.

pslacerda1 · 2025-10-09T23:57:09Z

Oh, I can see better now. Is it better now?

A current problem is the reuse the function name, how to declare a new command with the same name? The old behavior was to override the previous command in favor of the newer cmd.extend call.

Copilot

Pull Request Overview

This PR adds support for declaring new PyMOL commands with type annotations through a new new_command decorator. The change replaces the old declare_command function with enhanced functionality for command argument type parsing and documentation extraction.

Introduces a new new_command decorator with improved type annotation support
Adds comprehensive test coverage for various command parameter types
Implements automatic documentation parsing from function parameters

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
testing/tests/api/test_commanding.py	New comprehensive test file for command declaration functionality
testing/tests/api/commanding.py	Removes old `declare_command` tests that are replaced by new test file
modules/pymol/commanding.py	Implements new `new_command` decorator with enhanced type parsing and documentation features
modules/pymol/cmd.py	Updates import to use `new_command` instead of `declare_command`

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-10T22:34:36Z

modules/pymol/commanding.py

-
-    def _parse_list_str(value):
-        return shlex.split(value)
+                raise pymol.CmdException("Invalid boolean value: %s" % value)


The error message should use f-string formatting for consistency with similar errors elsewhere in the function.

Suggested change

raise pymol.CmdException("Invalid boolean value: %s" % value)

raise pymol.CmdException(f"Invalid boolean value: {value}")

Copilot · 2025-10-10T22:34:37Z

modules/pymol/commanding.py

+                    except:
+                        found = False
+                if not found:
+                    raise pymol.CmdException("Union was not able to cast %s" % value)


The error message should use f-string formatting for consistency with similar errors elsewhere in the function.

Suggested change

raise pymol.CmdException("Union was not able to cast %s" % value)

raise pymol.CmdException(f"Union was not able to cast {value}")

Copilot · 2025-10-10T22:34:37Z

modules/pymol/commanding.py

+        elif isinstance(type, str):
+            return str(value)
+
+        raise pymol.CmdException(f"Unsupported argument type {type}")


The error message is misleading. The variable type here is a type annotation, not a value type. Consider using {type!r} or a more descriptive message like 'Unsupported type annotation {type}'.

Suggested change

raise pymol.CmdException(f"Unsupported argument type {type}")

raise pymol.CmdException(f"Unsupported type annotation {type!r}")

Copilot · 2025-10-10T22:34:37Z

modules/pymol/commanding.py

+        while True:
+            i += 1
+            if tokens[i].string == "def":
+                while tokens[i].string == "(":


This loop will run indefinitely if it encounters a '(' token. It should use an if statement instead of while, or include proper bounds checking to prevent infinite loops.

Suggested change

while tokens[i].string == "(":

if tokens[i+1].string == "(":

This loop will run indefinitely if it encounters a '(' token. It should use an if statement instead of while, or include proper bounds checking to prevent infinite loops.

Maybe the "(" happens not immediately after the "def", in fact there is the function name in between, and I always saw them in the same line. Pretty sure that that tokens[i+1] doesn't suffice to skip the full function name. This is a while cursor is not parenthesis increase i+=1.

Oh, I'm wrong. While loop and if clause are equivalent here same there as after "def" always comes the function name and then "(".

I.e. they are the same.

I'd be happy if you @JarrettSJohnson could write or ask someone to write some code challenging the current implementation?

Copilot · 2025-10-10T22:34:37Z

modules/pymol/commanding.py

+                name = tokens[i].string
+                name_line = tokens[i].line
+                i += 1
+                while not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"):


This loop can cause an IndexError when accessing tokens[i+1] if i reaches the end of the tokens list. Add bounds checking: i+1 < len(tokens) to the condition.

Suggested change

while not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"):

while i+1 < len(tokens) and not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"):

This is a read only function. If it is broken, may be easier to rewrite than to fix.

This loop can cause an IndexError when accessing tokens[i+1] if i reaches the end of the tokens list. Add bounds checking: i+1 < len(tokens) to the condition.

I don't know, but seems that if you are inside a function declaration, the current token is a name, and your code is valid Python, the next token sometime it will happen to be a ":".

My mom always says to me to think before to talk. As you can see, she is correct right now as usual.

Copilot · 2025-10-10T22:34:38Z

modules/pymol/commanding.py

-        _self.help_sc.append(name)
+
+        inner.__arg_docs = parse_documentation(function)
+        _self.keyword[name] = [inner, 0,0,',',parsing.STRICT]


The spacing is inconsistent with missing space after the first comma. Should be [inner, 0, 0, ',', parsing.STRICT] for better readability.

Suggested change

_self.keyword[name] = [inner, 0,0,',',parsing.STRICT]

_self.keyword[name] = [inner, 0, 0, ',', parsing.STRICT]

JarrettSJohnson · 2025-10-10T22:37:40Z

Oh, I can see better now. Is it better now?

A current problem is the reuse the function name, how to declare a new command with the same name? The old behavior was to override the previous command in favor of the newer cmd.extend call.

How often do we see this in the wild, though? Keeping the same behavior sounds fine to me though I could be convinced with alternatives (logging when new_command overrides, etc...)

pslacerda1 · 2025-10-11T00:38:02Z

Of course this isn't a real problem! I is relevant only extend and new_command extending... i'll probably simply use unique names.

Is wonderful this Copilot feature, i'm more than amazed. My talent being replaced by a machine, i'm also very worried. Today i'll check into the review.

pslacerda1 · 2025-10-11T01:05:28Z

Okay, Copilot it is like smarter and dumber than anyone.

JarrettSJohnson · 2025-10-11T01:31:04Z

It can (usually) be wrong about a lot of things. Feel free to just ignore suggestions that you don't agree with, especially if it's easily refutable.

pslacerda1 · 2025-10-11T01:45:08Z

Oh Jarrett, I do my best on PyMOL, i care about it even more than my master thesis which I'm researching. But I'm just a developer finding where I can contribute the most.

(I talk lot of bullshit also, and i'm unfortunately a bit weird)

I want to say that i like your suggestions and considerations.

JarrettSJohnson · 2025-10-11T02:48:56Z

Master's thesis is important to focus on!😂 PyMOL will be here for a longer while ;)

pslacerda1 · 2025-10-11T03:10:07Z

Did you saw? I just talked nonsense! My thoughts are weaker in english.

I use PyMOL as a tool to study protein-ligand and protein-short peptide systems. Computer predicted hotspots suggests probes where ligands or peptides should overlap in order to enhance biological affinity or binding efficiency to their targets. I use PyMOL at various steps through my XDrugPy plugin.

pslacerda1 · 2025-10-11T03:45:56Z

This action (merge yours into mine, master to master) remove this pull-request from the list. Is it a Github bug?

pslacerda1 · 2025-10-14T14:24:19Z

My account were shadow banned because of the shared link! I'm back now. Do you think I can move to the command help? Maybe just pretty printing the function signature including docstring and showing on the (HTML?) console? Would this introduce a library?

Edit: would this require a library?

pslacerda1 · 2025-10-27T11:56:17Z

@JarrettSJohnson what do you think about the current version? Is it ready to merge?

JarrettSJohnson · 2025-10-29T18:57:16Z

modules/pymol/commanding.py

+        elif isinstance(type, str):
+            return str(value)
+
+        raise pymol.CmdException(f"Unsupported argument type annotation {type}")


So what are the consequences of a function without proper type annotations? Do they just not run? And if there's a bug in this implementation, is the command also just expected to not run?

This is supposed to be comprehensive and support most cases a typical command line plugin require. The cmd.extend would not be deprecated and may be used as a fallback.

If there is some bug in this implementation, we just add more tests and fix it. Did you saw something bugged on this implementation I'm unaware of?

JarrettSJohnson · 2025-10-29T19:32:46Z

modules/pymol/commanding.py

+
+        raise pymol.CmdException(f"Unsupported argument type annotation {type}")
+
+    def parse_documentation(func):


docstring/annotation. I assume this returns a dict[param_name] = param_comment ?

JarrettSJohnson · 2025-10-29T19:35:59Z

modules/pymol/commanding.py

+
+        raise pymol.CmdException(f"Unsupported argument type annotation {type}")
+
+    def parse_documentation(func):


Also, given the tests, this only retrieves comments within the def(...) scope? I am not sure if I've ever come across parameter documentation this way--this probably should be noted somewhere.

def func( # some value x: int): -> int return x * x

But

def func(x: int): -> int :param x: some value return x * x

is what i'm more familiar with.

We could also move the docstring for immediately before the declaration, so parameters came after docstring, if you like...

This is very good in my opinion because would tidify docstring with parameter documentation and typing. But tooling would not get it (e.g. integrated documentation in IDEs).

Python isn't the most syntactically flexible language but I like it plasticity enough...

I'll probably drop the documentation for now. But in my opinion we show use some strong convention, so we can generate rich documentation for new style command line plugins.

JarrettSJohnson · 2025-10-29T20:12:45Z

modules/pymol/commanding.py

        # Inner function that will be callable every time the command is executed
        @wraps(function)
        def inner(*args, **kwargs):
-            caller = traceback.extract_stack(limit=2)[0].filename
-
+            frame = traceback.format_stack()[-2]
+            caller = frame.split("\"", maxsplit=2)[1]


Having all of this being called every time a command runs makes me a little nervous, especially if these commands are done in a loop. A lot of this info is just redundantly calculated?

I added an attribute to direct assess the original unwrapped function. It's just cmd.foo.func() instead of cmd.foo(). Both works the same, but the first works faster.

pslacerda1 · 2025-10-30T19:53:43Z

I did a toy benchmark and it took 5 seconds of my machine. Pretty fast and enough for me.

import traceback
def c():
    caller = traceback.extract_stack(limit=2)[0].filename
    caller == "asdasodas"
def b():
    for i in range(1_000_000):
        c()
def a():
    b()
if __name__ == "__main__":
    a()

But also introduced an assessor to the original non-wrapped function so plugin developers can reduce any slowness caused because the cmd.new_command decorator. Again, they always can use the non deprecated cmd.extend.

pslacerda1 · 2025-11-04T03:10:19Z

@JarrettSJohnson Can we get this PR in 3.2, please? The previous implementation didn't worked well.

Should I recover the "commentstrings"? :-)

JarrettSJohnson · 2025-11-11T18:27:59Z

Hi, Pedro, I'm getting ready to merge your PRs, for tests that are failing, can you fix those that are fixable in this PR. Ones that depend on fixes from other PRs can just be commented out and marked as TODO.

pslacerda1 · 2025-11-11T22:42:41Z

I really don't know if there a useful purpose on typing.Any, the idea is that is a generic type that could match arbitrary values. But this one and Optional[] suffers the same problem, under-specification followed by poor implementation (Optional/None has no implementation yet). So we can pretty much say that this PR is an incomplete implementation in this regard.

If you think it is ok, I may dedicate time to finish Optional[] and maybe Any!

Again, any general scheme I didn't thought before can be made by @extend instad of @new_command.

… then I skipped.

pslacerda1 · 2025-11-12T19:56:53Z

Hi @JarrettSJohnson @speleo3, I think we can give a go on this @new_command as is and then we tailor as necessary while us refactor old plugin code in pymol-scripts-repo? What do you think?

JarrettSJohnson · 2025-11-12T20:51:53Z

Cannot merge this until your tests pass. Seems like something(s) not compatible with Python 3.10.

pslacerda1 · 2025-11-12T21:21:18Z

This code use the package strenum, which provides an implementation of StrEnum while Python 3.11 doesn't come yet.

JarrettSJohnson · 2025-11-12T21:22:53Z

Can you provide a workaround for 3.10?

Oh, you patched. One sec.

pslacerda1 · 2025-11-12T21:24:57Z

Is it alright? Or better to skip with pytest.mark.skip()?

Currently all tests will run but this one will fail because require Python 3.11 or the package strenum.

JarrettSJohnson · 2025-11-12T21:26:21Z

Can you mark it the test with python > 3.10?

pslacerda1 · 2025-11-12T21:41:29Z

i'm noob now,
wait just a moment...

pslacerda1 · 2025-11-12T22:40:48Z

I'm less noob now. But don't know what is the cause of failure just by observing logs.

speleo3 · 2025-11-12T22:42:34Z

Hi @JarrettSJohnson @speleo3, I think we can give a go on this @new_command as is and then we tailor as necessary while us refactor old plugin code in pymol-scripts-repo? What do you think?

Please excuse me if I say that I have mixed feelings about this implementation and don't want to endorse it. I assume it's an improvement over the already existing declare_command code, but still it's repurposing type annotations in a way that might be incompatible with how type annotation support by other tools is evolving. For me, type annotation is for type checkers (mypy) and IDE support (VS Code), and for that it's really powerful.

pslacerda1 · 2025-11-12T22:44:51Z

It is fully compatible with mypy! It is just another utility for type annotations, which currently is mostly for type enforcing and checks.

pslacerda1 · 2025-11-12T22:57:43Z

Maybe we should enforce mypy, it is a nice library. There are tools that observes running python programs and does annotation automatically, this could be a good start!

I just checked to be sure:

pslacerda1 · 2025-11-12T23:37:49Z

I never read too much of mypy, but I like it. The above/inline comment strings of arguments that would be a bit disruptive in terms of standard tools support. I think it is a win, but your opinion may diverge:

@cmd.new_command
def my_cmd(
  # Docs about argument foo: it ranges a lot
  foo: int = 0,
  # newer PyMOL-only documentation style about float bar numbers.
  bar: float = 0.0 # more docs for bar
):
  pass

I do not think very much of validation and beyond, I think more like a specific case to support modern command line prompts and get correct typing in the developer side instead of parsing everything always. As it is simple MyPy regular code, you may opt to do your own implementation in case this one becomes infeasible. However I see a good coverage of PyMOL-scripts-repo.

Just type enforcing and automatic casting (more like parsing).

pslacerda1 · 2025-11-13T00:20:25Z

This commit 3f49c4f is unnecessary. MyPy works just fine.

JarrettSJohnson · 2025-11-16T17:30:15Z

Does the above address your concerns? @speleo3

JarrettSJohnson · 2025-11-25T19:29:28Z

Maybe we should enforce mypy, it is a nice library. There are tools that observes running python programs and does annotation automatically, this could be a good start!

Why don't we? What benefit does your run-time checking provide that a statically enforced mypy doesn't?

pslacerda1 · 2025-11-27T00:00:29Z

Type conversion from text based commands. The user writes with a keyboard.

pslacerda1 · 2025-11-27T00:01:48Z

You have it at runtime and statically enforced if you like also.

pslacerda1 · 2025-11-28T03:32:48Z

I can tidy better my code if you want, like beautify the following.

        elif issubclass(type, Enum):
            if value in type:
                return type(value)
            else:
                raise pymol.CmdException(f"Invalid value for enum {type.__name__}: {value}")
        
        elif isinstance(type, str):
            return str(value)

But the real thing is that the subset of typing annotations supported by my implementation is fully supported statically by mypy. Of course is also supported at runtime doing type conversion and high level parsing.

You may diverge on the list or tuple implementation, but I think is fine enough also.

Any way, I focused on the function signature like a contract between plugin developers, PyMOL software and command line users. This I propose to be done with mypy based typing annotations.

pslacerda1 · 2025-11-28T05:45:55Z

its just type enforcing and data parsing...

if you become fast, we can move to autocompletion, documentation and maybe other features.

Speed... ok (a workaround), mypy strictness.... ok, python 3.10... ok. Would be awesome, but I'm reticent you don't like something about my implementation... if you talk we can maybe manage it.

JarrettSJohnson · 2025-11-28T16:46:37Z

Apologies, I can't reply quickly this week because it's Thanksgiving in the US and I'm traveling a bit.

Was mostly waiting for any further objections to the implementation from a previous reply, but haven't heard anything else. We can consider this was experimental for plugins, so I'm not too worried.

pslacerda1 · 2025-11-28T20:31:23Z

Happy traveling.

I'll document this experimental feature on the wiki soon.

After git suffering...

6df8dcb

Beter location for tests and renaming each testing function unique.

edebb39

JarrettSJohnson requested a review from Copilot October 10, 2025 22:33

Copilot AI reviewed Oct 10, 2025

View reviewed changes

Merge branch 'schrodinger:master' into master

51bbbbb

Add support to StrEnum

849d3b9

JarrettSJohnson reviewed Oct 29, 2025

View reviewed changes

pslacerda1 added 2 commits October 30, 2025 15:53

Merge branch 'schrodinger:master' into master

d921f8c

Removed comments on documentation

056a062

pslacerda1 mentioned this pull request Nov 10, 2025

Add support for >=, <= and == operators in selections #480

Open

pslacerda1 added 2 commits November 11, 2025 17:55

Merge branch 'schrodinger:master' into master

4c8e8f5

More support for extending PyMOL with functions

4b248c8

I think this is impossible in current implementation of @new_command,…

5a11eb3

… then I skipped.

Adding a testing library while not Python 3.11 yet

cf75cd2

pslacerda1 force-pushed the master branch from cca4ad1 to e38708c Compare November 12, 2025 21:52

Skip tests if current Python version is too old

b2784ed

pslacerda1 force-pushed the master branch from e38708c to b2784ed Compare November 12, 2025 22:01

Remove unused helper.

966f128

Moved testing functions to module level so they are findable by mypy.

3f49c4f

JarrettSJohnson merged commit 9c92399 into schrodinger:master Nov 28, 2025
12 checks passed

	raise pymol.CmdException("Invalid boolean value: %s" % value)
	raise pymol.CmdException(f"Invalid boolean value: {value}")

	raise pymol.CmdException("Union was not able to cast %s" % value)
	raise pymol.CmdException(f"Union was not able to cast {value}")

	raise pymol.CmdException(f"Unsupported argument type {type}")
	raise pymol.CmdException(f"Unsupported type annotation {type!r}")

	while not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"):
	while i+1 < len(tokens) and not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"):

	_self.keyword[name] = [inner, 0,0,',',parsing.STRICT]
	_self.keyword[name] = [inner, 0, 0, ',', parsing.STRICT]


		raise pymol.CmdException(f"Unsupported argument type annotation {type}")

		def parse_documentation(func):

Declaring new commands #470

Declaring new commands #470

Uh oh!

Conversation

pslacerda1 commented Oct 9, 2025

Uh oh!

JarrettSJohnson commented Oct 9, 2025

Uh oh!

pslacerda1 commented Oct 9, 2025

Uh oh!

JarrettSJohnson commented Oct 9, 2025

Uh oh!

JarrettSJohnson commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pslacerda1 commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pslacerda1 Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

JarrettSJohnson commented Oct 10, 2025

Uh oh!

pslacerda1 commented Oct 11, 2025

Uh oh!

pslacerda1 commented Oct 11, 2025

Uh oh!

JarrettSJohnson commented Oct 11, 2025

Uh oh!

pslacerda1 commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JarrettSJohnson commented Oct 11, 2025

Uh oh!

pslacerda1 commented Oct 11, 2025

Uh oh!

pslacerda1 commented Oct 11, 2025

Uh oh!

pslacerda1 commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pslacerda1 commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JarrettSJohnson commented Oct 9, 2025 •

edited

Loading

pslacerda1 commented Oct 9, 2025 •

edited

Loading

pslacerda1 Oct 11, 2025 •

edited

Loading

pslacerda1 commented Oct 11, 2025 •

edited

Loading

pslacerda1 commented Oct 14, 2025 •

edited

Loading

pslacerda1 commented Nov 11, 2025 •

edited

Loading

JarrettSJohnson commented Nov 12, 2025 •

edited

Loading

pslacerda1 commented Nov 12, 2025 •

edited

Loading

pslacerda1 commented Nov 12, 2025 •

edited

Loading